Skip to content
This repository was archived by the owner on Dec 21, 2021. It is now read-only.

Conversation

timoxley
Copy link
Contributor

@timoxley timoxley commented Apr 12, 2019

You may have noticed the following error being logged when running the integration tests:

Error: WebSocket is not open: readyState 2 (CLOSING)

image

https://travis-ci.com/streamr-dev/streamr-client-javascript/jobs/191090350#L2869-L2884

The above gets logged during running the current integration tests, yet the test suite does not fail because no test is actually listening for the client 'error' event at the time it occurs. It seems to be caused by the test suite itself not waiting for the publish action to finish before disconnecting the client. It appears the message does fail to be published.

Note the code below from the current master branch:

if (this.isConnected()) {
const streamMessage = await this.msgCreationUtil.createStreamMessage(streamObjectOrId, data, timestamp, partitionKey)
return this._requestPublish(streamMessage, sessionToken)
} else if (this.options.autoConnect) {

createStreamMessage is async, so it's entirely possible that the client will fully or partially disconnect between the first isConnected check and createStreamMessage resolving. This causes the _requestPublish to try writing to the disconnecting websocket, which in turn triggers the above error event, which isn't caught by anything.

This PR makes publish re-check the client is connected try to reconnect if autoConnect is set, or fail with an error "Disconnected before publish" otherwise.

Also adjusted the tests that initially exhibited this error to await their operations before continuing.


Update: Noticed similar behaviour for subscriptions: if you call client.disconnect() on subscribed, and the subscription had resend options, the client will cough up a similar error event as it tries to write to a closing websocket:

  console.error src/StreamrClient.js:229
    Error: WebSocket is not open: readyState 2 (CLOSING)
        at WebSocket.send (/Users/timoxley/Projects/streamr/streamr-client/node_modules/ws/lib/websocket.js:322:19)
        at Connection.send (/Users/timoxley/Projects/streamr/streamr-client/src/Connection.js:108:25)
        at send (/Users/timoxley/Projects/streamr/streamr-client/src/StreamrClient.js:559:29)
        at <anonymous>
        at process._tickCallback (internal/process/next_tick.js:189:7)

Solved this in 1fbd711 by simply ignoring resend requests if the client is no longer connected. A more general solution should probably be found.

@timoxley timoxley requested a review from hpihkala April 12, 2019 09:37
@timoxley timoxley force-pushed the disconnect-while-publishing branch from 43a6d48 to 01bde93 Compare April 12, 2019 09:37
@timoxley timoxley dismissed hpihkala’s stale review April 13, 2019 05:41

Changes addressed in e2e9a19.

@timoxley timoxley requested a review from hpihkala April 13, 2019 05:41
@timoxley timoxley force-pushed the disconnect-while-publishing branch from d1eed1b to e4829af Compare April 15, 2019 07:59
Copy link
Contributor

@hpihkala hpihkala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, only added a comment to your comment.

@timoxley timoxley merged commit 5d37621 into master Apr 23, 2019
@timoxley timoxley deleted the disconnect-while-publishing branch April 23, 2019 06:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants