fix(timeout): wrap customError with TimeoutError

[kwonoj]

Description:

This PR updates behavior of timeout when specify errorToSend for custom timeout message. Since custom timeout message can be anything (literally any), if caller does not specify it as timeouterror, when it's thrown it does not deliver context of error thrown. This PR checks type of error, wraps into timeout error to have additional context to debug.

with given example snippet
Rx.Observable.never().timeout(10, 'customerr').subscribe(console.log.bind(console));

Before

D:\github\rxjs\dist\cjs\scheduler\AsyncScheduler.js:45
            throw error;
            ^
customerr

After

D:\github\rxjs\dist\cjs\scheduler\AsyncScheduler.js:45
            throw error;
            ^
TimeoutError: customerr
    at new TimeoutError (D:\github\rxjs\dist\cjs\util\TimeoutError.js:17:26)
    at NeverObservable.timeout (D:\github\rxjs\dist\cjs\operator\timeout.js:30:17)
    at Object.<anonymous> (D:\github\rxjs\dummy.js:3:23)
    at Module._compile (module.js:573:32)
    at Object.Module._extensions..js (module.js:582:10)
    at Module.load (module.js:490:32)
    at tryModuleLoad (module.js:449:12)
    at Function.Module._load (module.js:441:3)
    at Module.runMain (module.js:607:10)
    at run (bootstrap_node.js:382:7)

Related issue (if exists):

[coveralls]

Coverage remained the same at 97.686% when pulling 3fb4715 on kwonoj:fix-timeout-customerr into 6cf7296 on ReactiveX:master.

[coveralls]

Coverage remained the same at 97.686% when pulling 3fb4715 on kwonoj:fix-timeout-customerr into 6cf7296 on ReactiveX:master.

[jayphelps]

What about instead checking if it's a string? if yes, wrap, if no, use as-is. Otherwise if someone provides their own object that doesn't extend Error, it would log TimeoutError: [object Object]

[kwonoj]

That's good point. One concern about allowing custom object is in that case it won't have stack trace to trace down still which this PR try to avoid for custom errors. Maybe instead of allowing error to be any kind limit its type to Error would be more simpler approach? (but this causes breaking changes to interface).

[jayphelps]

I don't fully understand the use case for providing an error instance. Why would you want to do .timeout(10, new SomeError())? Instead, wouldn't you really want to pass the class itself, to be instantiated when there is an error? .timeout(10, SomeError)? IMO that (along with the overload of providing just a string) is the more ideal API, to have the correct stack.

[kwonoj]

I agree. It's kind of compromise I could think of without deep consideration. In here, I think requirement to achieve are

• allow pass custom err, either string or other object
• wrap it as error, to have stack trace

So in case of caller deliver its own error object directly, it won't generate stack trace as expected. Any suggestion to solve both of those?

[mattflix]

Hi @jayphelps and @Blesh, I'm coming to this a bit late, but I think entirely the wrong decision has been made here.

I've read all the previous discussion (about Error instances and stack traces) and, in my view, a key point has been missed: That the Rx library should really have no opinion about what an error value is, or what it might be used for. To the library, any error value should be regarded as an opaque value to simply be passed along to Observer#error() as-is.

Imbuing the library with the strong opinion that the Observable#timeout() operator should/must throw an actual Error instance created in context (so the "correct" stack trace is captured) is much too limiting a position for the library to take, in my view.

Indeed, Observable#timeout() isn't much more than a specialization of Observable.throw(), which also takes "any" and will also not capture the "correct" stack if used as follows:

Observable.throw(new Error("hi"));

But, the point of these operators is not to capture the stack (property or improperly, which may also be a matter of opinion or use-case) or to necessarily aid in debugging -- the point is to allow the caller to supply the error value that they want. The caller may not wish to use Error instances (I know I've written code where this was useful). The caller may not care about capturing any stack (and may really rather avoid paying that cost).

In short, the old Rx API got this right the first time. Error values should be opaque to the library. The caller knows what they are doing. Let them do it.

That said, if we really want to address the "improper call stack" issue (for scenarios where the caller does want to use an Error instance to aid in debugging), and if you're committed to making a breaking change here anyway, then allow Observable.throw() and Observable#timeout() both to accept accept a function to be called in the context of the throw or timeout. The function can construct an Error (or even some other type of contextual error information) there, returning it for use as the value to be passed along to Observer#error() (again, the library having no opinion about the value).

In the meantime, I strongly suggest you back out this breaking change, and leave Observable#timeout() the way it was. The current change will definitely break working code for, IMO, no good reason, and (in some cases) add an unavoidable performance penalty (by unconditionally allocating an extra Error object and capturing a stack trace in cases when none is wanted).

[jayphelps]

Indeed, Observable#timeout() isn't much more than a specialization of Observable.throw()

They are mostly unrelated. Observable.throw is for creating a lazy observable which only does one thing: call observer.error() with your provided error value.

.timeout(x) is an operator which takes the source observable and mirrors it, but if the source fails to emit anything after the provided ms, it errors with a timeout.

So it has to throw some error for the timeout meaning three options: Require devs provide an exception class to throw, use our own internal exception class, or a combination of the two. We used to support a combination of the two, but we're not happy with the way it worked, and we don't want to let our bikeshedding hold back the v5-final release, so we made the call to remove the ability to customize the exception, and instead just us our own consistently. That doesn't mean we'll never add it back or more likely do something different, but better. We just know we weren't happy and it's better to make the breaking change now cause we actually add new arguments back to the operator later without it being a breaking change. FWIW other Rx implementations don't accept a custom exception either AFAIK, but it's true we don't want to blindly follow everyone else.

There was the second point of discussion which is creating the error object at the operator callsite so we have better stack traces in the event of a timeout, that is seemingly unrelated to whether or not we accept a custom error class or not cause the argument still stands for either option. We discussed the pros vs cons of creating that instance at the callsite vs. lazy, and generally came to the consensus that the developer experience outweighs the (in practice) very small hit of allocating the object we may or not not use.

I can totally listen to reason, but I'd need more guidance than saying we did it for no good reason.

[mattflix]

Let me correct myself on two points: (It was late. :)

1. Sorry, by "specialization" , what I meant was that the usage of the .throw() and .timeout() operators is similar (or should be, IMO) with regard to the supplied error value. In both cases, the caller creates and supplies the error value in the context of the operator's instantiation rather than the operation's later execution. This is sometimes a problem (esp. when creating an Error instance and desiring an execution-relative call stack), but not necessarily so.

2. I was wrong about the old Rx API. The old API, at least Rx4, despite what I said, does throw an Error instance from .timeout() (with .name set to "TimeoutError"). So, Rx4, too, has an opinion about what the error value should be in this case. Yuck.

I think the way .timeout() works is a mistake, in both the previous Rx versions and also now the "improved" version in Rx5. Rx should have no opinion about any error value. Rx should not assume or require that any error value is an instance of Error (after all, Javascript does not). If I want to time-out my streams using a string, number, or other object as the error value, I should be able to (without Rx getting in my way with "helpful" wrappers or assumptions).

So... perhaps not in this first release of Rx5, but if we are interested in improving the situation with regards to the inability to create the error value in the context of the .timeout() operator's (or even the .throw() or similar operator's) execution, we might consider having such operators accept an "error value creation" callback function as I described earlier, used to defer the creation of the error value (be it ultimately an Error instance or something else).

My main point I suppose: Having .throw() forward the error value supplied to it as-is while having .timeout() do something entirely different with the error value supplied it seems rather inconsistent and, to me, is confusing.

[KrishMunot]

Can you allow pass custom err, either string or other object and then pass the param as a trace?

[kwonoj]

@KrishMunot would you mind elaborate bit more then pass the param as a trace?

[benlesh]

Closing in favor of #2172

[lock]

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.