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

Retry operator did not re-trigger onListen/onSubscribe side effect #69

Closed
lestathc opened this issue Feb 22, 2017 · 4 comments · Fixed by #75
Closed

Retry operator did not re-trigger onListen/onSubscribe side effect #69

lestathc opened this issue Feb 22, 2017 · 4 comments · Fixed by #75

Comments

@lestathc
Copy link

As described in ReactiveX retry operator, current observable should be resubscribed once an error occurs.

But existing implementation does not do that.

@frankpepermans
Copy link
Member

There's an important gotcha to consider here,

In Dart we have 2 Stream types, single-subscription and multi-subscription.

For multi-subscription, there is no issue, we simply resubscribe when onError triggers,
on the other hand single-subscriptions cannot be resubscribed to (it would throw).

For the latter, either we simply swallow the Error, pretty much the current retry implementation, or we let retry throw right away, with a proper message saying resubscription is impossible here

thoughts?

@frankpepermans
Copy link
Member

or we let retry throw right away, with a proper message saying resubscription is impossible here

Here I mean, we throw even if no onError occurs, the retry operator simply should not be allowed on a single-subscription Observable

@brianegan
Copy link
Collaborator

Yeah, another tricky difference between the two implementations!

That's an interesting idea -- perhaps the best way would be to add an assert clause at the earliest possible point that would inform the developer that retry only works with multi-subscription streams.

That way we can "fail fast" and inform the developer of the mistake, rather than swallowing the developer error.

@lestathc
Copy link
Author

Hi, @frankpepermans

For multi-subscription stream, the onListen side effect will only be trigged at the first time it is listened.
See: https://dartpad.dartlang.org/b25b41c71f035fd61cb16b4a9a6e3fab

So, I think either document that the retry won't trigger the onListen side effect, or change the api to something like:

retry(Stream streamCreator()) {
   ...
   onError: (_) {
      streamCreator().listen(streamController.add, ...)
   }
}

Then streamCreator will be used to restart side effect (like sending http request).

How do you think?

brianegan added a commit that referenced this issue Feb 26, 2017
brianegan added a commit that referenced this issue Feb 27, 2017
@brianegan brianegan mentioned this issue Feb 27, 2017
brianegan added a commit that referenced this issue Mar 1, 2017
* Fixes #69: Create Retry factory that allows a stream to be retried N number of times.

* Fix controller not being properly closed

* Fix single-subscription bug

* Simplify Defer implementation, add single-subscription check as well
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

Successfully merging a pull request may close this issue.

3 participants