Python 3.7 Support & Task Queue Refactoring (again) #86
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
Fix indentation Fix typo
…ain) Expose keyword arguments of `websockets.serve`, resolves #83 Remove subprotocol selection function workaround Use InternalError instead of SignalingError where appropriate Add internal _TaskQueueState enum Mark a task as 'done' when it has been handled A task queue cannot be closed any longer, only cancelled When a task queue is cancelled, all enqueued tasks are also cancelled Add task queue join method which blocks until all enqueued tasks (including the currently active task) have been handled Try to join the task queue when closing a client's connection Fix close the connection as early as possible after the client's handler returned
Codecov Report
@@ Coverage Diff @@
## master #86 +/- ##
=========================================
Coverage ? 77.48%
=========================================
Files ? 9
Lines ? 1528
Branches ? 190
=========================================
Hits ? 1184
Misses ? 281
Partials ? 63
Continue to review full report at Codecov.
|
lgrahl
force-pushed
the
py37-support
branch
2 times, most recently
from
fc06571
to
c8ea6a8
Compare
The CLI tests also fail when running Python 3.4 locally. But the error that's being spit out makes absolutely zero sense. Since the CLI works perfectly fine in 3.4 when running it by hand and it only happens in 3.4, let's ignore this for now.
dbrgn
approved these changes
Sep 3, 2018
|
Sadly, there are still a bunch of errors when running the client tests. I want to finally track those down or we'll end up with yet another refactoring. |
Dead clients always have an associated protocol instance. That instance should eventually take care of cleaning up the connection. However, with the approach prior to this change, a connection may become closed before the protocol instance realises it. Thus, the protocol instance would never have the chance to clean up after itself. This might have been the cause of memory leaks (#52).
Move event tests into the TestServer class Add missing docstring to tests
…der immediately closes afterwards
Fix only cancel if the coroutine turns out to be a future Close (actual) coroutines in the task queue
Multiple tasks can be resolved with an exception at once. The previous code did handle only one exception, resulting in errors bubbling up
Add possibility to access log records that were recorded during a test Raise an exception in case an error has been logged in any test Add a filter for specific log messages that will not raise an exception
Add a test that checks that the server handles a disconnect correctly when sending a ping. Add another test that does the same when waiting for a pong.
Add a test that ensures a disconnect during receive is handled correctly Add a test that ensures a disconnect during send is handled correctly Add a test that ensures a disconnect while handling a task is handled correctly
…er in the event loop
|
New error to investigate when running the saltyrtc-client-js tests: And when stopping the server: |
Add error when enqueuing a task in an already cancelled task queue Few logging improvements
This prevents a race condition that happens when client A has started to disconnect but client B was still able to start the enqueue procedure. This can happen since the enqueue procedure itself is async and not sync.
This prevents yet another race condition that can happen when the exception from ServerProtocol.handle_client is being moved back into the event loop to be caught by ServerProtocol.handler. Another client would be able to pick up the PathClient instance and enqueue further tasks.
Closed
This ensures that no further task enqueues can be made when the connection is about to be closed (triggered by another client or by the server itself)
We do our own keep-alive pinging already and the currently exposed API of websockets is too inflexible for this purpose.
Formerly, all uncancelled tasks have been treated as if they are done. This resulted in exceptions when trying to fetch the result (or exception) from the task for logging purposes. Now, all tasks are cancelled if they are not already cancelled or not done. Tests have been added for cancelling active and queued tasks as well as coroutines since this wasn't adequately tested previously.
Add a test for relayed messages enqueued towards the responder before the initiator dropped the responder. Add a test for relayed messages enqueued towards the first initiator before the second initiator dropped the first initiator.
Formerly, it was possible that a client enqueues a message towards another client that had already been queued to be dropped. Add a test for a relayed message enqueued towards the responder after the initiator dropped the responder.
I don't like returning None...
Formerly, it was possible that a client, who is currently in the process of being dropped, could still forward messages to other clients. In order to fix this, we have to store the running task loops on the client, so the receive (and forward) loop is being cancelled once the client is being dropped.
Add a test for dropping a client which should not trigger a 'disconnected' message Replace `authenticated` flag with `ClientState` on `PathClient` Slightly more robust interaction between `Path` and `PathClient` More robust interaction between `Server` and `ServerProtocol` Update tests
Resolves #91
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.
Changes:
timeoutclose code (3008), see Add timeout close code saltyrtc-meta#150To do:
asyncio.gatherinstead ofasyncio.waitinServerProtocol.handle_client. Decision: No sinceasyncio.waitallows to provide more details in the log.asyncio.gather,asyncio.waitandasyncio.wait_for(regarding cancellation behaviour). Update: Moved into Revise cancellation of futures #87.Two errors, two warnings, one exception (when closing the server) leftjoin_task_queue)After merge, prepare a release.
Resolves #70. Resolves #79. Resolves #80. Resolves #81. Resolves #82. Resolves #83. Resolves #91.