-
Notifications
You must be signed in to change notification settings - Fork 762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Asyncpong #983
Open
bubbleboy14
wants to merge
20
commits into
master
Choose a base branch
from
asyncpong
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Asyncpong #983
+154
−118
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
…d()s w/ data instead of cb; WrappedDispatcher uses (experimental) buffwrite()
…asses handleDisconnect() to buffwrite()
…reate_dispatcher(), which passes it to WrappedDispatcher
… to reconnector()
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.
This PR consists of a few things, with the general intention of improving asynchronous writes and reconnect behavior.
TL/DR: fixes #976 and #942 and #974
background
Running asynchronously (with rel dispatcher), I noticed a couple things.
Firstly, I was seeing an error (SSLEOFError) that occasionally occurred in the course of a write, in particular while sending a PONG (hence the name of this branch). I actually think I saw some ticket about this a while ago, but wasn't able to reproduce it at the time.
Secondly, I noticed that my on_reconnect callback wasn't being fired, and that reconnects weren't reliable - I had to handle some cases in an on_error handler.
reconnects
The reconnect-related fix is in WrappedDispatcher: the problem was that reconnect() (by way of timeout()) wasn't passing True (as reconnecting kwarg) to setSock(); I modified reconnect() and timeout() to address this - problem solved.
async writes
I figured this was happening due to my asynchronous usage of the socket - it was probably trying to write when there wasn't room in the write buffer or writes weren't available for some other reason. For reference, WebSocket.send_frame() (in the _core module, on line 340) is where an individual frame is sent.
The fix is to write asynchronously in custom dispatcher / async mode. I modified WebSocket._send() (line 562) to use the send() function on the dispatcher. DispatcherBase now has a send() function, which just calls _socket.send() (as WebSocket._send() did previously - so there is no change with the default sync dispatcher). WrappedDispatcher.send() uses dispatcher.buffwrite(), which is available in the latest version (0.4.9.17) of rel. The WebSocket constructor now takes a dispatcher kwarg, and saves it on self.
Back in the _app module, in WebSocketApp, create_dispatcher() takes a handleDisconnect kwarg, which is passed to WrappedDispatcher. Also in run_forever(): handleDisconnect(WebSocketConnectionClosedException, bool(reconnect)) lambda is passed (as handleDisconnect kwarg) to create_dispatcher() (which WrappedDispatcher.send() passes to buffwrite()); and setSock() passes the dispatcher to the WebSocket instance.
why the _dispatcher module?
I moved DispatcherBase, Dispatcher, SSLDispatcher, and WrappedDispatcher to the new _dispatcher module so that the _core module could non-circularly import DispatcherBase and WrappedDispatcher for the dispatcher kwarg type hint in the WebSocket constructor.
status
This is still something of a work in progress. I've been testing this branch, and for my use case it's performing better than the current master branch, but further testing would be a good idea.
Please test with your use case and LMK how it goes!