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

WebSocketSubject cannot be retried. #1466

Closed
benlesh opened this issue Mar 14, 2016 · 7 comments
Closed

WebSocketSubject cannot be retried. #1466

benlesh opened this issue Mar 14, 2016 · 7 comments
Labels
bug Confirmed bug help wanted Issues we wouldn't mind assistance with.
Milestone

Comments

@benlesh
Copy link
Member

benlesh commented Mar 14, 2016

RxJS version: 5.0.0-beta.2

Code to reproduce:

let socket = new WebSocketSubject('ws://badurlhere');

socket.retryWhen((errs) => errs.switchMap(() => Observable.timer(1000))
  .subscribe(::console.log);

Expected behavior:

connection fails and then retries once a second.

Actual behavior:

connection fails and then UnsubscriptionError is thrown, subsequent subscriptions do not work

Additional information:

This apparently has to do with some changes to Subject and unsubscription error handling. I'm not sure though. We should definitely add some tests around this scenario, because it's important. WebSocketSubject was supposed to be reusable.

I found this will trying out the multiplex method in production for the first time. :\ Bummer.

@benlesh benlesh added bug Confirmed bug help wanted Issues we wouldn't mind assistance with. priority: high labels Mar 14, 2016
@rudolf
Copy link

rudolf commented Mar 23, 2016

I'm going to have a stab at this, I think I've identified the cause, but I'll write a failing test case to isolate it.

rudolf added a commit to rudolf/rxjs that referenced this issue Mar 23, 2016
Relates to ReactiveX#1466

Each time a multiplex subject unsubscribes, it sends the `unsub` message to
announce this to the server. If, however, the underlying socket closes, the
multiplex subject still tries to send it's `unsub` message over an already
unsubscribed subject which throws an `UnsubscriptionError`
@rudolf
Copy link

rudolf commented Mar 23, 2016

I created a failing test case and a fix:
rudolf@9fd31cc

When I apply the fix to my own test environment it solves the problem, but the test still fails for some reason. Any ideas?

The problem is each time a multiplex subject unsubscribes, it sends the unsub message to announce this to the server. If, however, the underlying socket closes, the multiplex subject still tries to send it's unsub message over an already unsubscribed subject which throws an UnsubscriptionError.

The current fix check's the underlying socket's ready state with self.socket.readyState < 3 before sending an unsub message, but it might also be possible to check self.isUnsubscribed.

@benlesh benlesh modified the milestone: 5.0.0 release Apr 29, 2016
fs123 added a commit to fs123/rxjs that referenced this issue May 31, 2016
It works since 5.0.0.beta.8, but there is no test which ensures this for the future.

related ReactiveX#1466
fs123 added a commit to fs123/rxjs that referenced this issue May 31, 2016
It works since 5.0.0.beta.8, but there is no test which ensures this for the future.
+ fixed tslint errors

related ReactiveX#1466
@fs123
Copy link
Contributor

fs123 commented May 31, 2016

It looks, that the bug does no longer exists since beta 8. See added test in PR 1738 above.

@benlesh
Copy link
Member Author

benlesh commented Jun 1, 2016

I created a failing test case and a fix: skaapgif@9fd31cc

@skaapgif did you already PR your fix and test for the unsub message?

benlesh pushed a commit that referenced this issue Jun 1, 2016
* test(WebSocketSubject): add test for multiplex in combination with retry

It works since 5.0.0.beta.8, but there is no test which ensures this for the future.

related #1466

* test(WebSocketSubject): add test for multiplex in combination with retry

It works since 5.0.0.beta.8, but there is no test which ensures this for the future.
+ fixed tslint errors

related #1466
@benlesh
Copy link
Member Author

benlesh commented Jun 1, 2016

Closed by #1738

@benlesh benlesh closed this as completed Jun 1, 2016
@rudolf
Copy link

rudolf commented Jun 1, 2016

I was still awaiting feedback, but @fs123's test is very similar and more thorough. I was able to reproduce the error without calling retry(1) though.

@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Confirmed bug help wanted Issues we wouldn't mind assistance with.
Projects
None yet
Development

No branches or pull requests

3 participants