Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
naggie opened this issue May 15, 2018 · 24 comments
Closed

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

naggie opened this issue May 15, 2018 · 24 comments

Comments

@naggie
Copy link

naggie commented May 15, 2018

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:

setTimeout(connect, reconnectDelay);

...but now the reconnect is not triggered:

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
Copy link
Author

naggie commented May 16, 2018

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
Copy link
Owner

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
Copy link
Author

naggie commented May 17, 2018

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
Copy link
Owner

Got it. Will investigate that scenario.

Thanks for reporting!

@naggie
Copy link
Author

naggie commented May 18, 2018

No problem. Thanks!

@usefulthink
Copy link

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
Copy link

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

@ernierasta
Copy link

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
Copy link

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
Copy link

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
Copy link

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

@haizz
Copy link

haizz commented Jun 25, 2018

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

@jobelenus
Copy link

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
Copy link

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

@jobelenus
Copy link

jobelenus commented Jul 10, 2018 via email

@dirkmoors
Copy link

+1

@borrascador
Copy link

borrascador commented Jul 21, 2018

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
Copy link

jobelenus commented Jul 21, 2018 via email

@tab00
Copy link

tab00 commented Jul 23, 2018

Please make it reconnect after a disconnect.

@tab00
Copy link

tab00 commented Jul 23, 2018

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
Copy link
Owner

Should be fixed in latest published version.

Thanks for your patience and reports!

@masad-frost
Copy link

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

@pladaria
Copy link
Owner

pladaria commented Oct 3, 2018

@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
Copy link

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

Feeling good today, @pladaria!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests