Because you said you were interested #46
Open
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
"ERR" in case of bad type has become more specific "WRONGTYPE"
Depending on the type of problem creating a connection (DNS resolution vs timeout vs ...), EventMachine may create a connection and then report failure via a callback, or it may throw an exception immediately. This commit fixes our handling of the second case, during reconnects. Initial connection may still throw.
Reconnect exceptions
Expose script and sha, provide method to ensure script is loaded
If an assertion is placed in a callback, but the errback is invoked, the assertion never happens, the test should fail, but it passes. Attach a lot of errbacks. Some broken tests are revealed.
This commit reworks much of em-hiredis to deal properly with several issues which we have come across in the last few months, including: Outstanding responses were tracked per client, but are actually per connection - a manual reconnect (calling close_connection) on the connection previously cleared the queue of pending responses, even though some of the responses might still be awaiting processing by eventmachine, getting requests and responses out of sync, with disasterous results. The pending responses are now tracked per connection. Failing to issue (for example) the PASSWORD or SELECT commands when creating a new connection did not abort the connection, leading the client to become connected but all reads and writes to either fail (authentication) or take place in the default db (select). These commands are now issued and their responses checked, failure counts as a connection failure and triggers a reconnect attempt. Over a long period of time the feature set had grown considerably, as well as the amount of edge case handling. This lead to a BaseClient class with a large set of concerns, complex state modelled as multiple booleans and a confusing interface for configuration and manual reconnection. The initialisation problem outlined above was solved incrementally, but the cost of following the logic was very high - when we were bitten by the response matching problem, the decision was taken to rework the code and seperate concerns as much as possible. This has led to: - No inheritance of pubsub connections and clients from regular ones - Connection management, inactivity checking, command issuing and queuing and state management (of active subscriptions) being seperated as appropriate. Considerable emphasis was placed on testing and producing a testable client, given the core importance of this library to the Pusher system. Seperation of concerns considerably aided the unit testing of components and unit testing in turn allowed the testing of many previously difficult to test cases, particularly in terms of connection initialisation ordering and failure. Some existing features have been removed. Their re-implementation should not be onerous if they are judged to be important by other uses of the client. This was available inconsistently, not when using the suggested helper methods for constructing clients, and not when reconfiguring a client. As there were two competing methods of configuration and URI was universal, it was kept at the expense of the individual values method to simplify the interface. Like pubsub, the MONITOR command switches the redis connection to what is essentially a different protocol - standard commands can no longer be issued, as their responses cannot be matched. It has been left out of the re-implementation as we did not have a use case for the programmatic use of MONITOR. It would not be difficult to re-introduce, and if this is desired, it should be done as a third connection/client class pair because of the "protocol differences" outlined here. em-hiredis offers a "best effort" approach to redis subscriptions. Because connection loss is abstracted from the user, messages may be lost at any time, so the "success" of a subscription does not offer any particular guarantees. Additionally, the redis pubsub protocol does not provide for notifying of the failure to process a subscribe or unsubscribe command. In order to simplify the code and in light of these limitations, the deferrable responses from subscribe and unsubscribe commands have been removed. Clients which wish to track the state of their subscriptions may still bind to the "low level" events and match them to their own requests - this also promotes the idea that the user should be wary of disconnections, because events are emitted for re-subscription after reconnection, making this common case more prominent. We suggest a bump of version to 1.0.0. This change bakes a large number of incremental feature additions and error handling cases accumulated throughout the 0.x.y series in to a model which is built around their existance from the start.
After 3: 1, 2, 3... We all hate implicit return!
Guards against potential stack overflows and arity limits when subscribed to huge numbers of channels
The default values for the hashes are set to be `[]`
This fixes a race condition when we unsubscribe and then receive a message from redis. In that case, the subscription would be recreated.
If there is no channel in the subscriptions object and we try to unsubscribe a proc from it, it will throw an error.
…tion-present Ignore unsubscribe by proc when no subscriptions exist for channel
Change resubscribe batch size to 1000
Ping init
Adding a non-optional positional arg after an optional one broke things. If two agrs are specified, they are assumed to be the two non-optionals (first and third), meaning the second can't be specified.
Fixes for Ruby 2.4 and broken tests
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.
I'm not sure a clean merge is possible. All the words I remembered writing are in the commit message of 3828688
Sadly, it appears that I wrote the commit message in markdown, forgetting that
#is a comment, not a title in git commit messages. Hence there are titles missing, which makes the text a little confusing :(