removing flush_and_error from on_error handler#533
Closed
chrishamant wants to merge 1 commit into
Closed
Conversation
Contributor
Author
|
reading some of the other issues re: 1.0 it seems this offline_queue may be going way of dodo anyway - so this may be moot... |
Contributor
|
@chrishamant given all of the people that rely on offline_queue it isn't going away, just being split from this library into a pluggable connection manager. So it definitely still needs to work right wherever it lands. |
Contributor
|
Thx a lot for your work @chrishamant ! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
There are few pull requests (#463 , #362 and #280 (maybe more)) which have been around a while which both attempt (in slightly different ways) to allow for persistence of commands in the offline_queue after a connection error. I'm honestly in support of either case - this pull request is yet another way of handling this.
Looking at the code I noticed that 'on_error' will call 'connection_gone' after it returns from emitting the Error (implication being that the user is listening for the error event). The 'connection_gone' method calls 'flush_and_error' also (it was already called in 'on_error' so it is kind of redundant I'm guessing). If the first call to 'flush_and_error' is just removed as done in this pull request - the command_queue and offline_queue will still be around on the client when the 'error' event is fired which gives the user time to buffer up the commands from these queues in any way they see fit while handling the error event (kind of moving the solutions in #463 and #362 into userland). Implication of this means that the top level 'error' event is fired before the individual command errors. I can conjure some hypothetical scenarios where this might be a problem (maybe someone started listening for 'error' only after getting error on command?) - but I don't think it would be an actual problem in practice.
Just an idea...