Fix handling of token error when first connecting. #986
Conversation
Removed complex and apparently unnecessary logic on error protocol message handler. No tests broken by this.
Soft-unwrapping catches assertion failures more gracefully.
Reconnection after DISCONNECTED and failing to establish a connection due to token error follow similar but slightly different logics. It's not worth trying to abstract it. Now, per RTN14b, of connection establishment failure: "If the attempt to create a new token fails, or the subsequent connection attempt fails due to another token error, then the connection will transition to the DISCONNECTED state, and the Connection#errorReason should be set. (If no means to renew the token is provided, RSA4a applies [ie. moves to FAILED])"
Test that we actually went through CONNECTED, instead of going directly to FAILED (that would be RTN14b). Also, don't assert on the error message. Asserting on the code is enough.
... not FAILED. Also, make it faster.
|
Well, it looks like one does not simply remove a bunch of lines.
This is so false I don't know how I reached that conclusion originally. I've fixed a bunch of them, but for some reason there are tests for unrelated things such as presence, etc. that do seem broken by this (since they pass on master), I don't know how yet. |
|
Ouch :( Is this still on yalls radar to resolve 🤞? |
|
@SpencerCDixon Yes, of course. |
|
Hey, @tcard sorry for being annoying but is there any chance you have a timeline on when this might get fixed and merged in? I have a few weeks of work porting over to Ably that is being blocked by this. We'd like to start beta testing the integration but if Ably clients can't get new tokens when folks computers are asleep that's a nonstarter. Most of our users keep our app running 24/7 and their computers are likely asleep overnight and would miss refresh token windows. Assuming our initial testing goes well we're going to be using your service with thousands of users and paying yall a lot of money 😄 Again, sorry to be annoying, I know you probably have a LOT going on just trying to get a handle on what I'll be working on next week and if I should be considering forking. |
|
@SpencerCDixon I didn't realize this was such a blocker for your development, since it's almost ready to be released. We'll prioritize getting this released as soon as possible. In any case, note that a workaround is possible by making a new connection each time the computer wakes up, passing the |
|
After deeper review, it seems this change is fine after all. Test failures were coincidentally caused by the flakiness of the test suite. |
|
Awesome, thanks so much! |
Removed complex and apparently unnecessary logic on error protocol message handler. No tests broken by this.
I first thought this was an issue with RSA4b, so I also added a missing test for it and some documentation.
Fixes #984.