pladaria / reconnecting-websocket

Reconnecting WebSocket. For Web, React Native, cli (Node.js)
MIT License
1.22k stars 197 forks source link

onclose behaviour has changed -- was it intentional? #60

Closed naggie closed 6 years ago

naggie commented 6 years ago

I noticed my application was failing to reconnect after the reconnecting-websocket rewrite. Upon investigation, I realised this seems to be because this library no longer reconnects when a websocket is intentionally closed by the server -- a close event is triggered but nothing else happens.

Previously, a graceful server-side close event would trigger a reconnect: https://github.com/pladaria/reconnecting-websocket/blob/e79fba552f236463adac04e1872f5e20e37d66a1/index.ts#L141

...but now the reconnect is not triggered: https://github.com/pladaria/reconnecting-websocket/blob/2e5d6f0406784da357e3213506c9b508c2b32991/reconnecting-websocket.ts#L392

The reconnect mechanism is triggered on error so I suspect this is an intentional change.

What do you think about adding a reconnectOnClose option, on by default?

Alternatively the docs could be updated to clarify this behaviour, and perhaps suggest manually binding a reconnect such as:

rws.addEventListener('close', () => rws.reconnect());

Although that seems to cause thrashing if the connection cannot be established, as reconnect calls _disconnect which calls the handlers again.

Thanks for the well written library.

naggie commented 6 years ago

In the mean time I've found a workaround that does not cause thrashing, however it does use private methods and attributes.

rws.addEventListener('close', () => rws._shouldReconnect && rws._connect());
pladaria commented 6 years ago

Hi,

Yes this behavior change is intentional. close now closes the connection. If you want to reconnect, there's a new method called reconnect.

Give it a try. Thanks!

naggie commented 6 years ago

To be clear, I'm not calling close() on the clientside. The stream is ending server-side but the client never reconnects. Reconnecting with reconnect() causes recursion.

pladaria commented 6 years ago

Got it. Will investigate that scenario.

Thanks for reporting!

naggie commented 6 years ago

No problem. Thanks!

usefulthink commented 6 years ago

Just ran into the same problem. If the server is terminated it will never trigger an error on the client-side (even when kill -9-ing the server-process) and thus never try to reconnect.

The workaround by @naggie seems to work fine in this case, would you be opposed to adding that (reconnectOnClose) as an option? I would be happy to prepare a PR for that.

StephenTG commented 6 years ago

Also encountering the same issue as naggie, currently sticking with v3.2.2 until this is resolved or usefulthink's suggested option implemented

ernierasta commented 6 years ago

The same here, v3.2.2 behaves how most of us wants. @usefulthink has a good idea. Or maybe even better - old behavior should be preserved. At the end - that is a reason why most of us use this library.

jobelenus commented 6 years ago

I'm on v3.2.2 and I'm seeing a similar(?) problem:

WebSocket connection to 'wss://...' failed: Error during WebSocket handshake: Unexpected response code: 503

  1. The page loads.
  2. The websocket loads just fine.
  3. It lasts for approximately 3.6 minutes until it dies off and tries to load a new one.
  4. It succeeds in loading a new connection.
  5. That new connection lasts for ~5 more minutes.
  6. It does not seem to think the connection is fully closed, it would seem, based on the devtools console (but I'm not entirely sure)?
  7. It does not open a new connection, my page is no longer updating
  8. I see the console error I pasted above

I am going to try the workaround from naggie.

jobelenus commented 6 years ago

OK, it seems that the methods used by naggie's fix do not exist in the v3.2.2 codebase (or anything similar). Is there something else I should be doing? Or should I be trying to trap an exception somewhere so the JS continues to run after the exception?

jobelenus commented 6 years ago

@pladaria I'm seeing a problem on 3.2.2 (details in previous comment from 2 days ago)

haizz commented 6 years ago

Is there any progress on this? Not reconnecting after server dropout totally devalues this library.

jobelenus commented 6 years ago

If the server drops a connection "normally" (e.g whether I'm local and restart it, or in production after a deploy and the server restarts) all connections get picked back up normally. That is not what I am seeing. I am seeing a JS error after a 503 is received which breaks any attempt at re-connecting.

ynotLeft commented 6 years ago

This is still an issue, I had to roll back to the version 3.2.2, which works as expected.

jobelenus commented 6 years ago

I'm on 3.2.2 and have experienced problems I documented above.

On Tue, Jul 10, 2018 at 4:26 AM, Anton notifications@github.com wrote:

This is still an issue, I had to roll back to the version 3.2.2, which works as expected.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pladaria/reconnecting-websocket/issues/60#issuecomment-403742122, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEPpVRyObJaQFOceYu0-SFvCuYSo8oHks5uFGU4gaJpZM4T_rX9 .

dirkmoors commented 6 years ago

+1

borrascador commented 6 years ago

Reporting my own experiences here. I am using version 4.0.0-rc5 and was able to resolve the problem following naggie and adding the following code to my ReconnectingWebSocket instance. @jobelenus I am pretty sure naggie's answer needs 4.0.0 to work, not 3.2.2

rws.addEventListener('close', () => rws._shouldReconnect && rws._connect()); // Use 4.0.0

And also confirming that the following solution suggested by @pladaria causes an infinite loop and crashes my site on testing.

rws.addEventListener('close', () => rws.reconnect());

I'm seconding a lot of people in hoping that naggie's code becomes the default behavior. Otherwise, making this a default option makes sense to me as well.

jobelenus commented 6 years ago

Thank you for the ping

On Sat, Jul 21, 2018 at 12:29 PM Jan Tabaczynski notifications@github.com wrote:

Reporting my own experiences here. I am using version 4.0.0-rc5 and was able to resolve the problem following naggie and adding the following code to my ReconnectingWebSocket instance. @jobelenus https://github.com/jobelenus I am pretty sure naggie's answer needs 4.0.0 to work, not 3.2.2

rws.addEventListener('close', () => rws._shouldReconnect && rws._connect());

And also confirming that the following solution suggested by @pladaria https://github.com/pladaria causes an infinite loop and crashes my site on testing.

rws.addEventListener('close', () => rws.reconnect());

I'm seconding a lot of people in hoping that naggie's code becomes the default behavior. Otherwise, making this a default option makes sense to me as well.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/pladaria/reconnecting-websocket/issues/60#issuecomment-406808027, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEPpUq_Mg_IbSDYWsiUqC-rH1TIoSueks5uI1b_gaJpZM4T_rX9 .

tab00 commented 6 years ago

Please make it reconnect after a disconnect.

tab00 commented 6 years ago

Thank you @naggie for the workaround.

My following adapted code is working well for me in the meantime:

      rws.onclose = function (event) {
        console.log(`WebSocket connection closed. event.code: ${event.code}, event.reason: ${event.reason}, event.wasClean: ${event.wasClean}`)

        if (rws._shouldReconnect) rws._connect()
      }
pladaria commented 6 years ago

Should be fixed in latest published version.

Thanks for your patience and reports!

masad-frost commented 6 years ago

It kinda matched my use case :cry:, then again https://xkcd.com/1172/

pladaria commented 6 years ago

@masad-frost Do you want that a server side close makes the websocket to not try to reconnect? I can add an option for that.

Please open a new issue explaining your requisites.

Thank you!

oytuntez commented 5 years ago

Aaaand, this commit saves me another hack to use RWS!

Feeling good today, @pladaria!