🐛 Fix connect lock issues #1
Conversation
alecgibson
force-pushed
the
reconnect-lock
branch
from
2e8e211 to
0b1f0b2
Compare
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@reedsy/reconnecting-websocket", | |||
| "version": "4.4.0-reedsy-1.0.0", | |||
| "version": "4.4.0-reedsy-1.0.1", | |||
There was a problem hiding this comment.
Do we really want to have version with reedsy if the package is called @reedsy/reconnectiong-websocket?
There was a problem hiding this comment.
I think it's nice: it encodes the upstream version we're on (which we can upgrade later if it also upgrades), and specifies our own internal reedsy version. It's what we're already doing in quill
There was a problem hiding this comment.
👍 We do not use this in all forked packages I guess we should unify it then
There was a problem hiding this comment.
how to install it？npm install @reedsy/[email protected]，always fail，not found
There was a problem hiding this comment.
Method 1 is not available. I tried to install according to the configuration in the document, but still failed to install. Method 2 is OK, but it is troublesome to configure "@reedsy:registry=https://npm.pkg.github.com" and "github token" of. npmrc by the company owner。
According to the prompts in the figure below, the following configuration failed in the window system
- "@reedsy/reconnecting-websocket": "git+https://[email protected]/reconnecting-websocket.git"
- "@reedsy/reconnecting-websocket": "git+https://github.com/reedsy/reconnecting-websocket.git"
- "@reedsy/reconnecting-websocket": "reedsy/reconnecting-websocket"
| @@ -376,6 +376,7 @@ export default class ReconnectingWebSocket { | |||
| .then(url => { | |||
| // close could be called before creating the ws | |||
| if (this._closeCalled) { | |||
| this._connectLock = false; | |||
There was a problem hiding this comment.
They said to add this._connectLock = false in close also
There was a problem hiding this comment.
I think they're incorrect, though. Adding it in the close() method adds a subtle bug. I've added a test case rapidly toggling connection to capture it. You should see if you do as they suggest, the test fails.
The issue is that if you set _connectLock = false in the close() method, you open up this case:
- Connect. We pass the
_connectLockcheck, so we start a_wait() - Now call
close()which in this case sets_connectLock = false - Call
reconnect(), which internally calls_connect(). Now, since we've reset_connectLock = falseinclose(), we pass the_connect()_connectLockcheck again, and set up a second_wait(), while the first hasn't finished - If we wait for these promises to resolve, we now create two WebSocket connections instead of one
This change fixes two issues described here: pladaria#165 1. It's impossible to reconnect after `maxRetries` is exceeded, because the lock is never released 2. It's impossible to reconnect if `close()` was called before the connection has been established This change adds tests and fixes as discussed in the above issue.
alecgibson
force-pushed
the
reconnect-lock
branch
from
0b1f0b2 to
080e6c6
Compare
|
Thanks for this PR, works perfectly. |
|
@shardick we haven't released it on NPM. You can either:
|
This change fixes two issues described here:
pladaria#165
maxRetriesis exceeded, because the lock is never releasedclose()was called before the connection has been establishedThis change adds tests and fixes as discussed in the above issue.