Drain lock #137
Merged
Drain lock #137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AmqpProtocol.close() was quite confusing due to its use of frame.payload as an argument to AmqpEncoder. The relationship between the 2 classes is now much more obvious.
Last user removed in previous commit.
It's used in all user-facing methods, but it's silently ignored by _write_frame()... so just drop it and let user code handle timeouts if needed.
With the previous commits, there's only one argument allowed by _write_frame. So use it explicitly in _write_frame_awaiting_response's signature.
Apart from open() where the code is *exactly* the same, the other 3 methods implemented a sub-par version, namely without any error handling should an exception be raised by _write_frame(). open() and close() set the 'check_open' argument to False because both methods modify the channel object's state.
There is a danger: if different tasks try to use methods from the same channel at the same time, there is a risk that publish() and basic_publish() frames get intertwined with unrelated frames, wreaking havoc. But right now, this is impossible because _write_frame isn't a real coroutine that yields to the main loop. It's simply a @coroutine-decorated function. Also, remove the 'is_open' check because _write_frame does it too.
Except the 2 publish methods, which call drain() explicitly after writing the 3+ (method, header and body) frames.
|
🦭 of approval |
RemiCardona
commented
Apr 11, 2017
| @@ -327,7 +333,7 @@ def send_heartbeat(self): | |||
| """ | |||
| frame = amqp_frame.AmqpRequest(self._stream_writer, amqp_constants.TYPE_HEARTBEAT, 0) | |||
| request = amqp_frame.AmqpEncoder() | |||
| frame.write_frame(request) | |||
| yield from self._write_frame(frame, request) | |||
There was a problem hiding this comment.
so calling yield from from a function does NOT work…
Back to the drawing board.
Will be needed in the next commit because write_frame will become a coroutine.
As of CPython 3.6.1, drain() is not coroutine-safe so put a lock around it to make sure it only ever gets called from a single coroutine at a time. http://bugs.python.org/issue29930 python-websockets/websockets#16
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Various spring cleanups and small refactoring that lead to adding a lock around calls to
Protocol.drain().See the links below as to why this is necessary: