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() only works with observables that return one event #721

Open
miracle2k opened this issue Oct 7, 2018 · 6 comments
Open

retry() only works with observables that return one event #721

miracle2k opened this issue Oct 7, 2018 · 6 comments

Comments

@miracle2k
Copy link
Contributor

If thesource stream of a Bacon.retry() returns first a Bacon.Next() event before it returns a Bacon.Error, retry will consider the stream finished. I would have expected that the stream will retry until there is a Bacon.End. The documentation does not talk about this at all.

Is there room to change the behaviour for Bacon 3?

@semmel
Copy link
Member

semmel commented Oct 9, 2018

As you suggested, let's assume for a moment that Bacon.retry waits for Bacon.End to consider the source a success. Then it would not ignore Bacon.Errors coming after the first Bacon.Next, but would in the middle of the stream re-generate the source. Thus an unknown number of duplicate Bacon.Nexts would appear in the callers context like this (E: error, X: end):

source#1:   --a--b--E-X
source#2:           --a--b--c-X
Bacon.retry --a--b----a--b--c-X

This repeating of events may be a problem for the consumer of the stream.

In Bacon 2.0 the behaviour is ..

source#1:   --a--b--E-X
Bacon.retry --a--b--X

... to swallow all errors, except the first ...

source#1:   --E--b--c-X
source#2:     --a--b--c-X
Bacon.retry ----a--b--c-X

... which gives us in a way retryable promises. Very nice!

@miracle2k
Copy link
Contributor Author

But am I wrong to think that a Promise (admittedly the main use-case) only have two possible outcomes:

1. [Bacon.Next(), Bacon.End()]
2. [Bacon.Error(), Bacon.End()]

So I was thinking, with the proposed changed behaviour, the user would still only ever see a a single Bacon.Next (because the stream cannot fail after receiving it.

What is the use case (in the current mode) for using Bacon.retry with an observable that returns multiple values?

@miracle2k
Copy link
Contributor Author

I guess someone could do:

Bacon.retry(
    Bacon.fromPromise(p).flatMap(x => something_that_fails())
)

Now the stream could be:

something_that_fails:   --a-b-c--E

And I suppose Bacon.retry() could return a, b, or c multiple times.

But wouldn't that be what the user wants? Otherwise, why not restrict the Bacon.retry to the promise itself?

@semmel
Copy link
Member

semmel commented Oct 10, 2018

But am I wrong to think that a Promise (admittedly the main use-case) only have two possible outcomes: 1. [Bacon.Next(), Bacon.End()], 2. [Bacon.Error(), Bacon.End()]`... ?

correct I guess

What is the use case (in the current mode) for using Bacon.retry with an observable that returns multiple values?

I can't see any use case. The disappearing trailing error events are a little "spooky".

Otherwise, why not restrict the Bacon.retry to the promise itself?

Yes I guess that would be the logical consequence.
The breaking change would be, that the source function in Bacon.retry should return a Promise and not an event stream. I think that would be useful.

Besides to .*toPromise() and Bacon.fromPromise(), Bacon.retry would be another interface point between Bacon event streams and promises.

I often wrap promises (and make them cancelable) with Bacon.fromBinder or Bacon.fromPromise. Then I treat them like single-valued Bacon.once streams to import them into my "reactive" Bacon event pipelines.

@miracle2k
Copy link
Contributor Author

Yes I guess that would be the logical consequence.
The breaking change would be, that the source function in Bacon.retry should return a Promise and not an event stream. I think that would be useful.

Sure, that's one option, but my point was that Bacon.retry could be useful given a source that yields multiple events, while having an unchanged behaviour for the promise case (provided I am not missing something). But as it stands, it is pretty much only useful in the promise case.

But if I want to retry an observable that has more than a single event, why wouldn't Bacon let me?

@semmel
Copy link
Member

semmel commented Oct 15, 2018

Ok I basically understand your issue, however I would still object against unreproducible multiple emission of events. (Those leading up to the error which eventually causes the repeats of the sequence).

I mean, a "successful" stream (without an error) consists of all events up to the Bacon.End event. And that is the result I want to have. In the best case without any retries. All the unsuccessful attempts before don't matter.

Now if, as in your case, just Bacon.End determines that the stream is actually a "success", since traveling backwards in time is not possible, one could also completely forget about the temporal sequence of events and delivery them at once collected in an array:

// a regular source function for Bacon.retry
var generate = cAttempts => Bacon.sequentially(100, [0, 1, 2, cAttempts < 2 ? new Bacon.Error("oops!") : 3]);
// helper function
var reduce = s => s.reduce([], (acc, x) => R.concat(acc, [x]));
Bacon.retry({source: R.compose(reduce, generate), retries: 2}).flatMap(Bacon.fromArray); 
// -> 0,1,2,3,<end>
Bacon.retry({source: R.compose(reduce, generate), retries: 1}).flatMap(Bacon.fromArray)
// -> <error>, <end>
source#1:   --a--b--E-X
source#2:           --a--b--c-X
Bacon.retry ----------------abc-X

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

2 participants