Error handling and fromPromise()

[ivan-kleshnin]

Let's compare error handling in RxJS and Kefir:

RxJS. Example 1

import {Observable as O} from "rxjs"

O.from([1, 2, 3]).map(() => {
  return x.y
}).subscribe(
  (x) => console.log("x:", x),
  (e) => console.log("e:", e),
)

e: ReferenceError: x is not defined
    at MapSubscriber.project (/Users/ivankleshnin/Sandboxes/rxreact/ex1.rxjs.js:4:10)

Kefir. Example 1

import K from "kefir"

K.sequentially(100, [1, 2, 3]).map(() => {
  return x.y
}).observe(
  (x) => console.log("x:", x),
  (e) => console.log("e:", e),
)

ReferenceError: x is not defined
    at /Users/ivankleshnin/Sandboxes/rxreact/ex1.kefir.js:4:10

As we see, RxJS mixes operational and syntax errors which I consider a very bad idea.
Both end up being handled in the same function. Kefir separates two error types. Only operational errors will be caught and handled by our custom (e) => ... lambda.

At least, both libraries show us the correct file, line and column.
Now let's take a look at Promise based streams:

RxJS. Example 2

import A from "axios"
import {Observable as O} from "rxjs"

O.fromPromise(A.get("http://localhost:8080/api/users/")).map(() => {
  return x.y
}).subscribe(
  (x) => console.log("x:", x),
  (e) => console.log("e:", e),
)

e: ReferenceError: x is not defined
    at MapSubscriber.project (/Users/ivankleshnin/Sandboxes/rxreact/ex2.rxjs.js:5:10)

The same behavior as in Example 1.

Kefir. Example 2

import A from "axios"
import K from "kefir"

K.fromPromise(A.get("http://localhost:8080/api/users/")).map(() => {
  return x.y
}).observe(
  (x) => console.log("x:", x),
  (e) => console.log("e:", e),
)

(node:4313) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): ReferenceError: x is not defined
(node:4313) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Now this one is broken. Kefir looses an error stack trace and the cause of our syntax error is lost.
For some reason, the promise "sucked" the error in. It's neither handled by error handler like in RxJS, nor escaped to the process like previously 😞 As a consequence, any promise-based stream in Kefir is a hell to debug right now.

I expected to get a process crash from sync errors in both Kefir examples.

[mAAdhaTTah]

For some reason, the promise "sucked" the error in.

Unfortunately, that's how promises work, and there's not really a whole lot we can do about that. I've run into the same problem, and I've mostly worked around it by not interoperating Promises with Observables. For example, I wrote a wrapper around XMLHttpRequest for a project (which I'll eventually extract into a separate library).

But yeah, that's just how they work. I don't know if there's a good way to get around that.

[ivan-kleshnin]

In any case you shouldn't lose a stack trace. This is clearly a buggy behavior.

import K from "kefir"

K.fromPromise(Promise.resolve("foo")).map(() => {
  return x.y
}).observe(
  (x) => console.log("x:", x),
  (e) => console.log("e:", e),
)

kefir.js (v1)

function fromPromise(promise) {
  ...
  var _promise = promise.then(onValue, onError);
  ...
}

produces

(node:4313) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): ReferenceError: x is not defined
(node:4313) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
    ... stack trace is missing ...

kefir.js (v2)

function fromPromise(promise) {
  ...
  var _promise = promise.then(onValue).catch(onError);
  ...
}

produces

e: ReferenceError: x is not defined
    at /Users/ivankleshnin/Sandboxes/rxreact/ex2.kefir.js:4:10 !!!
    ... stack trace is present ...

kefir.js (v3)

function fromPromise(promise) {
  ... 
 var onValue = function (x) {
    setImmediate(() => {
      emitter.emit(x); // this time .emit is not trapped inside promise .then and 
      emitter.end();   // behaves like in normal promise-less streams (see above)
    })
  };
  ...
}

I think v3 is what we need because we get a normal error reporting:

/Users/ivankleshnin/Sandboxes/rxreact/ex2.kefir.js:10
  return x.y;
         ^

ReferenceError: x is not defined
    at /Users/ivankleshnin/Sandboxes/rxreact/ex2.kefir.js:4:10

and syntax/operational errors are now separated.

Syntax errors can't be handled (they crash the process in Node as they should).
Operational errors can be handled (and mapped to whatever we want).

[rpominov]

The problem is that we have to execute some code inside then callbacks, and if this code throws (which could happen because of bugs in user code) Promises want to handle these exception. This is too aggressive from Promises, but we can't change that.

kefir/src/interop/from-promise.js

Lines 9 to 17 in db4ea0d

	let onValue = function(x) {
	emitter.emit(x)
	emitter.end()
	}
	let onError = function(x) {
	emitter.error(x)
	emitter.end()
	}
	let _promise = promise.then(onValue, onError)

A possible solution could be to minimize code that we execute in then callbacks. In particular only schedule execution of our logic using setTimeout(..., 0). This will change behavior of the library slightly though.

[mAAdhaTTah]

@ivan-kleshnin The only issue w/ v3 is you still end up without the stack trace, for better or worse.

This will change behavior of the library slightly though.

If we use setImmediate, I don't know as the difference will be enough to break the usage. I don't know how much people are relying on the specific microtask semantics of Promises like that when interoping w/ Kefir. If you had the Promise, and did one conversion into an Observable and one that didn't, the callbacks attached to each may fire in a different order. But I would argue they shouldn't rely on that and would be very uncommon.

If we're considering this a "bug", is there any way / desire to interop with a "Promiseish" library (e.g. Fluture or fun-task) that doesn't do this? I don't know how much that helps, but may be a useful workaround if you're trying to get away from this behavior.

[ivan-kleshnin]

The problem is that we have to execute some code inside then callbacks, and if this code throws (which could happen because of bugs in user code) Promises want to handle these exception.

Then you mix operational errors with syntax errors just like RxJS does. I don't think it's a good behavior.
But RxJS is at least consistent with that.

For Kefir, it won't be acceptable that

.map(faultyHandler)

in one case throws, and in another pipes an error to error handler in observe depending on some stream source 150 lines above.

the only issue w/ v3 is you still end up without the stack trace, for better or worse.

There is a normal stack trace. I showed the first line of it.

If we're considering this a "bug", is there any way / desire to interop with a "Promiseish" library (e.g. Fluture or fun-task) that doesn't do this? I don't know how much that helps, but may be a useful workaround if you're trying to get away from this behavior.

To be honest, I think try-catch is an antipattern. It conflates the errors of entirely different natures. In case of JS & AJAX it's: syntax errors + dynamic errors + connection errors + 404 NotFound + 500 ServerError... An absurd. Now promises make an emulation of try-catch. And observables make an emulation of promise behavior. So it's an emulation of an emulation of an antipattern.

The best decision IMO is to make catch catch only promise rejections. Not everything else.
The current promise behavior is really super-agresssive, paranoidal. Add an error swallowing to that and get the worse async behavior ever created. And now RxJS devs claim "they're going to swallow errors by default, like promises do". If their logic leads to this absurd behavior – their premises are false. One of the reason I'm leaving RxJS.

[rpominov]

If we use setImmediate, I don't know as the difference will be enough to break the usage.

Not sure about current support in browser, but we'll probably have to use the hack with iframes then, which always seemed silly to me. But I won't object too much, Most and Rx probably use that hack too so...

If we use setImmediate I think it will be OK to release this as a patch/minor version.

If we're considering this a "bug", is there any way / desire to interop with a "Promiseish" library (e.g. Fluture or fun-task) that doesn't do this? I don't know how much that helps, but may be a useful workaround if you're trying to get away from this behavior.

Yeah, I also don't know how this would help. It would be nice to have integration with these libraries, but we need an integration with Promises anyway.

Then you mix operational errors with syntax errors just like RxJS does. I don't think it's a good behavior.

The way promises work don't leave us much of a choice 😉
Promises do aggressive errors catching not us.

[ivan-kleshnin]

The way promises work don't leave us much of a choice 😉
Promises do aggressive errors catching not us.

At this moment, Kefir does agressive error catching. I'd like this to be changed if possible.
I showed that there is at least one way to change that, so I don't buy the argument "it's all promises, not us". How this should be done – with setImmediate or something else – it's up to tests and all possible considerations.

Interoperability with promises does not necessary mean we have to accept their patterns of thinking.
Errors that happen in Kefir's .map and other handlers belong to Kefir. The fact that they happen inside then buried somewhere within the library is an implementation detail (even if it's the only possible implementation). Promise-based Kefir streams should not differ drastically from other Kefir streams in their error handling – that's what I'm saying.

[Macil]

Adding a setTimeout(..., 0) means that the code would no longer execute within the current microtask queue, so the DOM may be rendered to the screen before the settimeout callback runs. I generally don't care about the order of different microtasks, but the fact that on modern browsers they all execute before the screen gets rendered is something I depend on often.

[ivan-kleshnin]

@agentme promises are async already. setTimeout won't make any difference to DOM rendering
because it's not sync -> async change. It's async -> async (with different catching scope)

I think you should show the code you mean, so we can test it with both approaches.

[rpominov]

Yeah, setTimeout may bring subtle issues, but if we use setImmediate we'll probably be fine.

[Macil]

@ivan-kleshnin Promises are async, but they're part of the microtask queue, so they get executed as part of the current task which the browser doesn't render during. Compare these two silly snippets of code in the console on a simple webpage:

function f() { document.body.style.backgroundColor = 'green'; setTimeout(()=>{ document.body.style.backgroundColor = 'white'; setTimeout(f, 100); }, 0); }
f();

function f() { document.body.style.backgroundColor = 'green'; Promise.resolve().then(()=>{ document.body.style.backgroundColor = 'white'; setTimeout(f, 100); }); }
f();

In modern browsers that correctly implement promises and microtask queues (this includes Firefox and Chrome at least, no idea about IE), then in the second one, you'll never see the page's background color rendered green. (Browsers are free to render the background color green briefly in the first example. Chrome tends to show it every single time, but Firefox skips it occasionally for me.)

This is a silly example, but I find it common to do things equivalent to this. Consider a showData function which has code like element.textContent = "Loading..."; dataPromise.then(data => element.textContent = data);. If the data was already loaded, then you don't want "Loading..." to flash on the screen for a single frame.

@rpominov One problem is that setImmediate is not implemented on Chrome (and isn't standardized so easily might not be in other or future browsers). It could be used if available. I wonder if the inconsistency in how errors get handled might surprise people, but I don't feel strongly about that since Kefir doesn't have any real existing story about handling exceptions.

[rpominov]

setImmediate is not implemented on Chrome

That bothers me too. We could use polyfills (like RxJs does for example), although I don't like the idea of having these hacks in codebase to be honest.

P.S.
Ha ha, RxJs 5 has a much nicer implementation of setImmediate, but we can't use it for our problem: https://github.com/ReactiveX/rxjs/blob/ab05c46826e26790adfe653e590a0c1a821994db/src/util/Immediate.ts#L16

[mAAdhaTTah]

There is a normal stack trace. I showed the first line of it.

My mistake, you're correct. I thought you'd lose part of the stack trace because you'd lose it's connection to the promise, but I realize now that's a devtools feature, not a promise error stacktrace feature, and actually would include the setImmediate as well (if we went that route).

Yeah, I also don't know how this would help. It would be nice to have integration with these libraries, but we need an integration with Promises anyway.

It would help insofar as it gives users another non-error-swallowing option. The issue is there aren't really libraries built on them, e.g. I don't know of an AJAX lib built on Fluture, and a quick google search didn't turn up anything obvious, so in all likelihood, if userland needs a Fluture-based AJAX lib, they need to write it themselves, at which point, they might as well just write the lib in Kefir.

So maybe it doesn't really help 😕

At this moment, Kefir does agressive error catching.

We're only "doing" that error catching because we're relying on Promise's default behavior, but I think we all agree with you that Promises doing this is... not great. @rpominov wrote a whole thing about it here. So the debate isn't if it's good or not so much as whether and what we can do about it.

One problem is that setImmediate is not implemented on Chrome

Wait, it isn't? I tested this in my browser console w/o issue: setImmediate(() => console.log('hello immediately')). Am I missing something?

If it's not, then yeah... we've got a bit of a problem, because as @agentme points out, the task would no longer be added to the same queue and the behavior changes.

[Macil]

@mAAdhaTTah Did you check for setImmediate while on github? Github has a polyfill for it. That's not a native function you're seeing.

Actually Firefox doesn't implement it either. ... I checked FF while on Github lol. setImmediate is actually specific to IE and recent Node versions.

[mAAdhaTTah]

I did not, but I guess whatever site I was on had it. Although the console said [native code], I do not see setImmediate on the Google homepage, for example.

That still leaves us without a good solution here without breaking / changing the semantics, unless we want to require a setImmediate polyfill...?

[ivan-kleshnin]

I thought you'd lose part of the stack trace because you'd lose it's connection to the promise, but I realize now that's a devtools feature, not a promise error stacktrace feature, and actually would include the setImmediate as well (if we went that route).

I tested in NodeJS and stack is fine there as well. Not sure what you mean by "devtools".

Anyway, guys I totally understand it's easier to say than done, so I rely on your decision here.

@mAAdhaTTah thanks for the link to fun-task. I didn't pay attention to this repo (yet) so I'll take my time to check it.

[mAAdhaTTah]

So to recap, here are our options:

1. Nothing, leave the code as is
   • Pros
     • No code changes are the best code changes
     • No chance of breaking userland code
   • Cons
     • Doesn't actually solve the problem
     • Error handling changes when interoperating w/ Promises
2. Use setImmediate to break out of the Promise stack
   • Pros
     • Least likely to cause userland breakage
     • Error handling is consistent
   • Cons
     • setImmediate isn't implemeneted on all browsers
     • Would require a polyfill
3. Use custom setImmediate-like behavior (e.g. postMessage) to break out
   • Pros
     • Same as setImmediate
     • No polyfill required
   • Cons
     • Kind of hacky
     • Bloats the code with a number of fallbacks, which we have to maintain and keep free of bugs
     • Essentially the same as shipping a polyfill ourselves
4. Use setTimeout to break out
   • Pros
     • No polyfill required
     • Definitely solves the problem in all browsers
   • Cons
     • setTimeout is queued on a different task queue, could cause behavioral differences, e.g. renders after DOM manipulation

Did I miss anything in the list? Anyone got any other potential options here?

[mAAdhaTTah]

I have a potential option: remove Promise interop altogether and push that out into a separate library where we can document the differences in behavior, or even multiple packages that handle it different ways, depending on consumer preferences.

[ivan-kleshnin]

Did I miss anything in the list?

Seems legit to me.

setImmediate isn't implemeneted on all browsers
Would require a polyfill

https://github.com/YuzuJS/setImmediate/blob/master/setImmediate.js

This polyfill is not tiny, but not super big either.

---

The option to remove Promise support from the core is also interesting. It seems there's not many bindings to them in Kefir. fromPromise and toPromise and that's all? RxJS and Most are coupled to Promises to a bigger degree.

[rpominov]

fromPromise and toPromise and that's all?

Yep.

Maybe we should deprecate fromPromise / toPromise and at the same time create a separate repository that implements it using setImmediate polyfill?

We could even leave toPromise, but it will be weird to have toPromise but not fromPromise.

[mAAdhaTTah]

Maybe we should deprecate fromPromise / toPromise and at the same time create a separate repository that implements it using setImmediate polyfill?

I dig this. If we're going to break one out to a separate repo, I'd prefer doing both to & fromPromise. Then we could make toPromise a function the consumer can use with thru.

[Macil]

Node.js's default unhandled promise rejection handler only shows the error message without its stack trace. You can make it show the whole stack trace by adding an unhandled promise rejection handler, which is probably a good idea if you're using promises at all regardless of Kefir:

process.on('unhandledRejection', error => {
  console.error('unhandled promise rejection', error);
});

Kefir.fromPromise(Promise.resolve()).onValue(() => {throw new Error('test error');});

There is a stalled bug report about node.js's default unhandled promise rejection handler being absolutely useless.

Chrome's console always allows seeing the full stack of errors from unhandled promise rejections.

Most of Kefir doesn't try to handle exceptions at all as it is, and promises are increasingly core to javascript with the popularity of async functions (using await someKefirStream.take(1).toPromise(); is an amazingly convenient thing in certain situations that ought to be recommended more so people know it's an option), so I'm a little against putting roadblocks in front of .toPromise() like deprecating it because of this. Maybe we should just put some notes in its description about the issue and recommendations in this thread? (We could include a link to a fromPromise module that uses a setImmediate or an equivalent workaround.)

[ivan-kleshnin]

@agentme the problem we discuss is not limited to stack trace. It's about inconsistency with error handling current fromPromise introduces. It won't be solved by changing the unhandledRejection handler.

As I showed above, we can get our stack back by replacing then(okHandler, failHandler) with then(okHandler).catch(failHandler) inside Kefir.fromPromise. But that won't remove inconsistency either.

Most of Kefir doesn't try to handle exceptions at all as it is

Exactly. And current fromPromise violates this expectation. It's like from a different RxJS-like library.

[mAAdhaTTah]

The issue with the current Promise interop is that this:

Kefir.fromPromise(Promise.resolve()).onValue(() => {throw new Error('test error');});

even throws an unhandledRejection error at all. It really shouldn't; once you're outside of "Promise land" (hur hur) because you converted to a Stream, errors should be in "Stream land" and behave according to Kefir's semantics.

(using await someKefirStream.take(1).toPromise(); is an amazingly convenient thing in certain situations that ought to be recommended more so people know it's an option)

I personally never use Kefir in that way; for me, if I'm using Observables, it's Observables all the way down. I use fromPromise more often to interop with other libraries, especially fetch, so I'm disinclined to see value in it. If you use it a lot, or find it convenient, that might be an argument for leaving it.

Maybe we should just put some notes in its description about the issue and recommendations in this thread? (We could include a link to a fromPromise module that uses a setImmediate or an equivalent workaround.)

Is this to suggest that you'd want to leave the current behavior in the library and also add the second package suggested above?

[ivan-kleshnin]

After reading this https://github.com/rpominov/fun-task/blob/master/docs/exceptions.md and reconsidering my programming experience, I'd like to add some additional info and code examples.

I come to the conclusion that error handling in streaming library is only useful in handling "Expected failures". "Unexpected failures", as noted, are impossible to handle except logging (remote HTTP "alerts" also fall into this category). And for logging, a singleton global event listener should be more than enough. And we have one in both Browser and Node.

Unfortunately for us, the authors of Promise A+ spec have chosen a different path and Promise.catch catches everything, including unexpected failures. The PRO of that is that it emulates a sync try-catch, the CONS is that it's honestly an absolutely broken behavior. Sync try-catch should have been applied only to userland throw, and not anything else, if we dig further...

How it should look like, being done correct? In the same way in both streams and promises – operational errors (expected failures) are stopped by catch, code errors (unexpected failures) are crashing the process / stop JS execution in case of browsers.

As I mentioned Task primitive, let me briefly add a few comments:

task.run({
  success(x) {
    // handle success
  },
  failure(x) {
    // handle expected failure <-- Yes! This is great!
  },
  catch(e) {
    // handle a bug <-- I woudn't support this feature. Bugs shouldn't be handled locally, i.m.o.
  },
})

To use Promise's second path for expected failures is a terrible idea. (R.Pominov)

Yes! But, actually, it may suddenly become a good one(!) if we:

• tested the library and can guarantee it doesn't throw on bugs because it has none
• don't rely on promise chains in app code (userland).

Then we can make a coresspondance between Promise.catch to Task.failure / Kefir <error>.

---

The following code demonstrates a fetch of blog posts (all at once, no pagination). Fetching is done in two steps: 1) fetch post ids 2) fetch posts if necessary:

// A is Axios, K is Kefir, R is Ramda

// intents: Object (Stream *)
let intents = {
  // HTTP
  fetch$: sources.state$.sampledBy(K
    .fromPromise(A.get("/api/posts/~/id")) // promise error -> stream error
    .map(resp => R.pluck("id", resp.data.models)),
    (state, requiredIds) => {
      let presentIds = R.keys(R.view(baseLens, state))
      let missingIds = R.difference(requiredIds, presentIds)
      return missingIds
    })
    .filter(R.length)
    .flatMapConcat(ids => K
      .fromPromise(A.get(`/api/posts/${R.join(",", ids)}`)) // promise error -> stream error 
      .map(resp => resp.data.models)
    ),
}

...

// action$ :: Stream (State -> State)
let action$ = K.merge([
  // Every such "channel" may encounter MANY errors of a SINGLE type (N chans <= N types)
  intents.fetch$.map(posts => {
    return function afterFetch(state) {
      return R.over(baseLens, R.mergeFlipped(posts), state)
    }
  }).flatMapErrors(err => {
    console.warn(`Request to "${err.response.config.url}" failed with message "${err.response.status} ${err.response.statusText}"`)
    return K.never() // can modify state to add alert box here!
  }),
])

Now if we make a code error like:

  fetch$: sources.state$.sampledBy(K
    .fromPromise(A.get("/api/posts/~/id")) // promise error -> stream error
    .map(() => x.y.z), // !!!

it crashes and don't end up being mixed with operational errors. It works this way only with setImmediate addon, of course. Otherwise, code errors are getting here:

  .flatMapErrors(err => {
    console.warn(`Request to "${err.response.config.url}" failed with message "${err.response.status} ${err.response.statusText}"`)
    return K.never() // can modify state to add alert box here!
  }),

we can't tell what is what, and things are getting out of control... Being tied to Promise.catch (as it is now) I'd choose to avoid Stream <error> channel at all, and, instead, marshal data manually as value events. If would require a lot of noisy if-else checks, but it would be the only sane option left (not counting Either).

[mAAdhaTTah]

I know I never got around to adding this package, but a queueMicrotask is coming to node and apparently browsers, although none of them have implemented it yet. Worth considering whether we want to change the fromPromise semantics in a 4.0.0 to use that after it lands, since the primary issue with setImmediate was that it was never specced into the language and so not widely adopted.