Incorrect error handling in .combine?

[rpominov]

Consider this example:

A:                ----1-----e-----2-----
B:                -------3-----4-----5--
combine(A + B):   -------4--e--5--6--7--

The 5 value after the error combined as 1 + 4. The 1 is the latest value from A, and combine uses it ignoring the fact that there was a error after it.

I think this behavior is not correct, combine should continue to emit errors on new values from B until there is a value in A. In other words third value should be combined from e and 4 which result in e. So the correct output should be:

combine(A + B):   -------4--e--e--6--7--

Although I am personally didn't use errors very much, maybe I'm missing something, and current behavior more useful than what I suggest? With the new behavior it will be tricky to achieve current one, if somebody will want to...

---

Same issue in Kefir https://github.com/pozadi/kefir/issues/54

[phadej]

Errors in properties aren't well specced indeed. I actually have no idea which behaviour is "the best", but your version seems more legit. Using FRP-semantics it should so, as after event e the current underlying value of A is Error(e) not Value(1).

I was thinking if it should be

combine(A + B):   -------4--e-----6--7--

i.e. error not re-pushed. But that behaviour is easy to recover from your version.

[rpominov]

Using FRP-semantics it should so, as after event e the current underlying value of A is Error(e) not Value(1)

Yep, I was thinking about this too, even plan to make it work this way in kefir https://github.com/pozadi/kefir/issues/55 (sorry for so many links to kefir :smile:)

[phadej]

No worries. I have my own FRP-ish library too, yet invested much less time into it. :)

[raimohanska]

Good points guys! In Bacon.js, Errors do not affect the state of a Property, so the previous valid value remains current. It might indeed make more sense to persist the error state too. This would also be more consistent with how Promises work.

Yet, I don't quite see the point of repeating the error each time in combine, so my guess at the "correct" behaviour for combine would be

combine(A + B):   -------4--e-----6--7--

How about that? :)

[raimohanska]

This might be one of the breaking changes to be included in version 0.8.

[phadej]

@raimohanska i though about it too, but you can go from

combine(A + B):   -------4--e--e--6--7--

to

combine(A + B):   -------4--e-----6--7--

but no other way around. So do we want to lose information on purpose?

[raimohanska]

You might also consider it incorrect to emit 2 errors while just 1 actually occurred.

On 21.2.2015, at 21.49, Oleg Grenrus notifications@github.com wrote:

@raimohanska i though about it too, but you can go from

combine(A + B): -------4--e--e--6--7-- to

combine(A + B): -------4--e-----6--7-- but no other way around. So do we want to lose information on purpose?

— Reply to this email directly or view it on GitHub.

[phadej]

error occured only once, but the other stream had an event. Consider:

A                           : -1-------3-
B                           : -0---2-----
combine(firstArgument(A,B)) : -1---1---3-

where

var firstArgument = (x, y) => x

There we do emit 1 twice because we don't know what firstArgument does. IMHO we shouldn't treat error values differently. I.e. what to do when:

A            : 1------e1-----
B            : 1---2-----e2--
combine(A+B) : 2---3--e1-??--

IF we go with the rule combine result is a first error occured in any of input streams, if any (I can't actually formulate it precisely). Then we run into other corner cases:

A            : 1------e1------3------
B            : 1---2-----e2-------4--
combine(A+B) : 2---3--e1-----e2---7--

IMHO it's surprising that we get e2, but later. (1)

---

I would like Bacon.js to move towards FRP ideas, where we have Behaviours which are time &rarr a functions, and Bacon.js Properties are something like Behaviour (Either Exception a) – in Haskell speak – with derivied semantics.

In menrva Signals don't have built-in Either inside. IMHO that makes semantics clearer. On the other hand, that is not as convenient, because error handling is explicit. Yet Bacon.js doesn't auto-catch exceptions (the issue which got me involved in this project ;), so error handling is semi-explicit anyway.

---

Sorry to bring this abstract mambojumbo, but IMHO the project would largely benefit from specified denotational semantics, so we can judge whether some behaviour is a bug or by design.

(1): Yet one could write down precisely that behaviour too, but it will need a notion of whether the underlying event stream has an event or not. i.e. it's simpler to emit event always when any of combined stream has an event, and leave the filtering to the user.

[rpominov]

To me it more intuitive to emit error multiple times. Also as @phadej pointed out it easy to transition from that behavior to single error, but not other way around.

But here is another problem, what if in more than one source last event was an error? E.g. we can agree that anyOperation(error1, value1) === error1, but what anyOperation(error1, error2) should return? I guess best we can do is anyOperation(error1, error2) === latestOf(error1, error2), and I actually think it good enough :)

[raimohanska]

I agree with your proposal for the multi-error case: just return the latest error. I guess that should be automatic anyways, if each Property (incuding the combined one) is supposed to persist it's latest value/error, right?

No point in arguing, but I still don't find it very intuitive to output a new error on each value, because the situation really doesn't change: the same error is still the current value of the combined property...

[rpominov]

After some thoughts of the case of multiple errors: maybe better solution is to produce array of all errors? In case of one error it would be an array with one element.

anyOperation(value1, value2) -> Next(resultValue)
anyOperation(error1, value1) -> Error([error1])
anyOperation(value1, error1) -> Error([error1])
anyOperation(error1, error2) -> Error([error1, error1])

What do you think?

[rpominov]

Draft of Kefir docs on this:

[raimohanska]

If you combine sources that are combined sources themselves, will your errors be arrays of arrays or errors? I'm afraid this will end up as a mess, at least from the type perspective. A more consistent solution would be, in my opinion, to have Error events always contain an array of errors. Otherwise your error handling code will have to do a lot of iffing to take different types into account.

[rpominov]

will your errors be arrays of arrays

Yes, but I see it as a necessary evil, and not a big issue. User can simply apply _.flatten() and get one level array.

A more consistent solution would be, in my opinion, to have Error events always contain an array of errors.

Interesting idea, but there is some problems: 1) It means more breaking changes 2) if user also use arrays as error objects, it will be handled specially, which probably undesirable 3) if we do _.flatten() in lib code we lose information of errors sources 4) there may be more, as it a big change...

[rpominov]

And if user don't want arrays as errors, he can add .mapError(arrayToDesirableType) after each .combine.

[raimohanska]

I wish we had a language with a type system :) In Bacon, the Error event currently carries an .error property for a single error. I'm thinking about extending this event object so that it can carry a single (latest) error in this property, but can also carry the full array of errors in another property. This way the current API would not break, but you could access the more refined representation from the new property. The onError method currently calls its callback with a single argument "error", and this could be extended to provide the more detailed state in a second argument. Similarly you could add another parameter in the Kefir error handlers too, right?

So, to summarize, don't break current API, but support a more detailed API in addition.

Does this make sense?

[rpominov]

This is totally makes sense, sounds like a very solid solution! Have to think about it for a while, though.

[raimohanska]

Please do, and point out any problems you might find. I gotta admit I didn't think very deeply on this. But this stuff gets more important in Bacon.js to if we adopt the convention of maintaining the latest error too, in addition to just maintaining the latest value.

[rpominov]

This approach have some problems indeed:

1. We'll have to add second argument not only to .onError callback, but also to all callbacks that accepts errors — in .mapErrors, .flatMapErros etc. + all future methods that work with errors.
2. We still lose information of errors sources, by putting errors into an array, and by doing flatten/concat when combining twice or more.
3. The problem we are trying to solve is in .combine method, would be nice if it was solved in this method not affecting other parts of the system.
4. The container chosen for additional information on error objects (array), and method of merging several errors (E(1, [1,2]) + E(3, [3,4]) = E(latestOf(1,3), concat([1,2], [3,4]), if I understand correctly) works for .combine. But if we decide to use this for some other method it might not work.

---

I came up with another solution: what if we add second combinator function for combining errors? By default it would be latestOf(errors...), but user will be able to replace the default behavior. It'll make 80% easy and 20% possible, as we can pass to errors combinator all information about errors, including time and source.

It might look something like this:

// new .combine signature
prop.combine(secondProp, combinator[, errorsCombinator])

// examples of errorsCombinator call
errorsCombinator(null, {time: 1428657860371, error: 'oops'})
errorsCombinator({time: 1428657860371, error: 'oops'}, {time: 1428657860372, error: 'oops'})

This looks like a nice solution for Kefir, because as for now it has basically one .combine-like method the .combine. For Bacon you will have to add this errorsCombinator to a lot of methods like .combineWith, .combineAsArray etc., which, I understand, not nice at all :(

---

Good news both solutions looks like not breaking, so if we don't have time to go full way now, or not ready to decide which way to go, we can only implement default behavior in .combine — latestOf(errors...), which already will be better than current behavior.