feat(error): Lazy Error.stack usage #2781

hermanbanken · 2017-08-04T13:23:24Z

Description:
The Observable.timeout method creates a TimeoutError on usage, before actually needing it. While this correctly sets the timeout's stacktrace to the use, greatly simplifying finding bugs, it also generates the stacktrace - with the current implementation - due to .stack being accessed.

Especially when using compiled languages like TypeScript with the source-map-support library this would cause expensive source map parsing (all while no error occurred yet). See for example this issue with ts-jest for more details.

This PR changes Rx' custom errors to access Error.stack lazily.

Another solution would be to just skip the this.stack assignment and rely on the proper inheritance of Error, not sure if that would work.

Related issue (if exists):

rxjs-bot · 2017-08-04T13:34:18Z

	Warnings
⚠️	commit message does not follows conventional change log (1)

	Messages
📖

(1) : RxJS uses conventional change log to generate changelog automatically. It seems some of commit messages are not following those, please check contributing guideline and update commit messages.

CJS: 3180.8KB, global: 590.1KB (gzipped: 108.8KB), min: 138.6KB (gzipped: 29.1KB)

Generated by 🚫 dangerJS

coveralls · 2017-08-04T13:37:38Z

Coverage decreased (-0.06%) to 97.68% when pulling ff69803 on hermanbanken:feature/errors-lazy into d2a32f9 on ReactiveX:master.

coveralls · 2017-08-04T13:44:25Z

Coverage decreased (-0.06%) to 97.68% when pulling ff69803 on hermanbanken:feature/errors-lazy into d2a32f9 on ReactiveX:master.

coveralls · 2017-08-04T14:06:12Z

Coverage decreased (-0.06%) to 97.68% when pulling dfcf919 on hermanbanken:feature/errors-lazy into d2a32f9 on ReactiveX:master.

kwonoj · 2017-08-04T18:50:15Z

We have actually changed our custom error (targeted next for now) to no longer try to mutate those directly (https://github.com/ReactiveX/rxjs/pull/2614/files#diff-cd4b33df1ab129e23dc2bc5c12d8369bR8)

which probably doesn't require this changes. This is interesting approach, but we could probably just cherry-pick those changes only from next instead (as I believe it won't be breaking?)

hermanbanken · 2017-08-05T19:45:12Z

Yes, those setPrototypeOf calls are fine as well! Good alternative. I don't think it is breaking: the same properties are exposed, the errors don't change.

There is some other stuff in there too though. A careful cherry-pick, including only the non-nexty changes, would be fine.

I can rebase this PR to such a cherry-pick on Monday.

hermanbanken · 2017-08-07T14:59:23Z

Never mind. The proposed changes in next are wrong: stack === undefined (at least when running mocha).

I added a test for this in the last commit. I had no idea Errors where so complicated in JavaScript. See for example https://gist.github.com/justmoon/15511f92e5216fa2624b#anti-patterns.

coveralls · 2017-08-07T15:05:08Z

Coverage decreased (-0.07%) to 97.667% when pulling bb068c5 on hermanbanken:feature/errors-lazy into d2a32f9 on ReactiveX:master.

kwonoj · 2017-08-07T15:12:47Z

If changes in next doesn't work as expected, I'd rather try to make legit changes for next and backport it into master instead of having 2 separate changes (as long as it's not breaking changes).

hermanbanken · 2017-08-07T15:15:10Z

Agreed... I'm writing some tests as we speak.

Unfortunately the PR #2614 is a little messed up because next is merged in back instead of rebasing. Therefore cherry-picking that cleanly is not easy. That complicates the backporting part...

coveralls · 2017-08-07T15:49:08Z

Coverage decreased (-0.04%) to 97.696% when pulling 809656c on hermanbanken:feature/errors-lazy into d2a32f9 on ReactiveX:master.

TypeScript 2.4 is more strict about assignability with generics, and in some cases (when calling a generic function) it will infer that the generic parameter is <{}> rather than the <T> that rxjs typically wants. Fix this by explicitly passing <T> in the cases where compilation fails.

Error.stack can potentially trigger expensive prepareStackTrace calls, Observable.timeout would crate a TimeoutError on use, before the actual error occurs. This eases tracing the location of the timeout, but with the current implementation, slows down the application when using many .timeout operators throughout the code.

coveralls · 2017-08-07T16:39:29Z

Coverage decreased (-0.07%) to 97.667% when pulling bb068c5 on hermanbanken:feature/errors-lazy into d2a32f9 on ReactiveX:master.

benlesh · 2018-03-30T22:01:37Z

I suddenly have more time to invest in RxJS.. because it's actually my job now...

I think this is something we're interested in doing. It does need more review, but it also needs refactored since files moved around.

cartant · 2018-05-30T01:50:55Z

@benlesh I've had a look at this PR with a view to getting it into a state in which it can be applied to v6. However, I'm not convinced that it's required. In fact, I think the current, v6 implementation should be preferred.

Using TypeScript 2.8.3, the following code:

class SomeError extends Error {
    private err: Error;
    constructor() {
        const err: any = super("some error");
        this.err = err;
    }
    get stack(): string | undefined {
        return this.err.stack;
    }
}
const s = new SomeError();
console.log(`has stack = ${Boolean(s.stack)}`);
console.log(`instanceof Error = ${s instanceof Error}`);
console.log(`instanceof SomeError = ${s instanceof SomeError}`);

outputs this:

SomeError has stack = true
instanceof Error = true
instanceof SomeError = false

Whereas this code:

class AnotherError extends Error {
    constructor() {
        super("another error");
        (Object as any).setPrototypeOf(this, AnotherError.prototype);
    }
}
const a = new AnotherError();
console.log(`has stack = ${Boolean(a.stack)}`);
console.log(`instanceof Error = ${a instanceof Error}`);
console.log(`instanceof AnotherError = ${a instanceof AnotherError}`);

outputs this:

AnotherError has stack = true
instanceof Error = true
instanceof AnotherError = true

Note that the implementation that uses this PR's mechanism returns false for instanceof SomeError and that both implementations provide the stack. For me, the current implementation works fine in the browser (Chrome Headless) and in Node (v8.11.2).

The PR's tests likely passed, as v5 was running them using TypeScript 2.0 and the change regarding the extension of built-ins was introduced in 2.1.

Here's a playground link if you want to try it out.

benlesh

Overall it looks good. I'm confused about the typings changes, we're building against TS 2.8, not 2.4 so I"m surprised there were any issues with the areas touched... We should keep those type changes to a separate PR.

benlesh · 2018-05-30T17:52:54Z

spec/util/EmptyError-spec.ts

+      observable.first().subscribe();
+    } catch (err) {
+      expect(err instanceof EmptyError).to.equal(true);
+      expect(err.stack).to.not.be.undefined;


.to.be.a('string') would be a more specific check here.

benlesh · 2018-05-30T17:53:23Z

spec/operators/timeout-spec.ts

@@ -33,6 +33,7 @@ describe('Observable.prototype.timeout', () => {
      throw new Error('this should not next');
    }, err => {
      expect(err).to.be.an.instanceof(Rx.TimeoutError);
+      expect(err.stack).to.not.be.undefined;


Same as the other spot. .to.be.a('string')

benlesh · 2018-05-30T17:53:41Z

spec/observables/dom/ajax-spec.ts

@@ -262,6 +262,7 @@ describe('Observable.ajax', () => {
    });

    expect(error instanceof Rx.AjaxError).to.be.true;
+    expect(error.stack).to.not.be.undefined;


Same .to.be.a('string')

benlesh · 2018-05-30T17:54:15Z

src/observable/FromObservable.ts

@@ -91,7 +91,7 @@ export class FromObservable<T> extends Observable<T> {
        return new FromObservable<T>(ish, scheduler);
      } else if (isArray(ish)) {
        return new ArrayObservable<T>(ish, scheduler);
-      } else if (isPromise(ish)) {
+      } else if (isPromise<T>(ish)) {


This change seems unrelated.

benlesh · 2018-05-30T17:55:02Z

src/operator/catch.ts

@@ -66,7 +66,7 @@ import { subscribeToResult } from '../util/subscribeToResult';
 */
 export function _catch<T, R>(this: Observable<T>, selector: (err: any, caught: Observable<T>) => ObservableInput<R>): Observable<T | R> {
  const operator = new CatchOperator(selector);
-  const caught = this.lift(operator);
+  const caught = this.lift<T>(operator);


This change seems unrelated.

benlesh · 2018-05-30T17:55:51Z

src/util/TimeoutError.ts

+  constructor() {
+    const err: any = super('Timeout has occurred');
+    this.err = err;
+    (<any> this).name = err.name = 'TimeoutError';


this as any .. we're trying to move away from this style of casing and move to as whatever

cartant · 2018-05-30T19:41:34Z

@benlesh I'm suggesting that this PR should be closed - i.e. not merged - as the current v6 implementation is better behaved. Do you not agree?

hermanbanken · 2018-05-31T06:03:06Z

Does the new implementation “touch” the stack? Eg access the getter/setter of the stack property? Because that was the actual problem I described. If it uses inheritance and does not access that field ahead of time then we should be fine without this PR and I would also suggest closing it.

cartant · 2018-05-31T06:09:47Z

@hermanbanken I don't believe it does, I did some checking of the __extends function that's in the emitted TypeScript implementation and I don't believe it does touch the stack property. I can double check, but I'm close to certain that it does not.

cartant · 2018-05-31T06:25:45Z

@hermanbanken Just did a quick perf test and can confirm that the current implementations - that call super followed by setPrototypeOf - don't touch the stack property.

benlesh · 2018-05-31T18:31:34Z

@cartant ... sorry, I missed your comment. Looking at this again, I believe you're right. Sorry, @hermanbanken. I really appreciate your effort here, but we have to close this one. I hope this doesn't put you off from further contributions. I really do appreciate the effort here! ❤️

hermanbanken force-pushed the feature/errors-lazy branch from 55f0a45 to ff69803 Compare August 4, 2017 13:24

hermanbanken changed the title ~~Lazy Error.stack usage~~ featOerLazy Error.stack usage Aug 4, 2017

hermanbanken changed the title ~~featOerLazy Error.stack usage~~ feat(error) Lazy Error.stack usage Aug 4, 2017

hermanbanken changed the title ~~feat(error) Lazy Error.stack usage~~ feat(error): Lazy Error.stack usage Aug 4, 2017

hermanbanken force-pushed the feature/errors-lazy branch from ff69803 to dfcf919 Compare August 4, 2017 13:52

kwonoj requested a review from benlesh August 4, 2017 18:50

kwonoj added the PR: Requires Further Review label Aug 4, 2017

hermanbanken force-pushed the feature/errors-lazy branch 3 times, most recently from 7b94eb9 to bb068c5 Compare August 7, 2017 14:57

hermanbanken force-pushed the feature/errors-lazy branch from bb068c5 to 809656c Compare August 7, 2017 15:19

evmar and others added 3 commits August 7, 2017 18:15

Add test if stack is set

756005a

hermanbanken force-pushed the feature/errors-lazy branch from 809656c to a38bfe6 Compare August 7, 2017 16:27

benlesh added Needs refactor for v6 and removed PR: Requires Further Review labels Mar 30, 2018

benlesh requested changes May 30, 2018

View reviewed changes

benlesh closed this May 31, 2018

lock bot locked as resolved and limited conversation to collaborators Jun 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(error): Lazy Error.stack usage #2781

feat(error): Lazy Error.stack usage #2781

hermanbanken commented Aug 4, 2017 •

edited

Loading

rxjs-bot commented Aug 4, 2017 •

edited

Loading

coveralls commented Aug 4, 2017

coveralls commented Aug 4, 2017

coveralls commented Aug 4, 2017

kwonoj commented Aug 4, 2017

hermanbanken commented Aug 5, 2017

hermanbanken commented Aug 7, 2017 •

edited

Loading

coveralls commented Aug 7, 2017

kwonoj commented Aug 7, 2017

hermanbanken commented Aug 7, 2017

coveralls commented Aug 7, 2017

coveralls commented Aug 7, 2017 •

edited

Loading

benlesh commented Mar 30, 2018

cartant commented May 30, 2018 •

edited

Loading

benlesh left a comment

benlesh May 30, 2018

benlesh May 30, 2018

benlesh May 30, 2018

benlesh May 30, 2018

benlesh May 30, 2018

benlesh May 30, 2018

cartant commented May 30, 2018 •

edited

Loading

hermanbanken commented May 31, 2018

cartant commented May 31, 2018

cartant commented May 31, 2018

benlesh commented May 31, 2018

feat(error): Lazy Error.stack usage #2781

feat(error): Lazy Error.stack usage #2781

Conversation

hermanbanken commented Aug 4, 2017 • edited Loading

rxjs-bot commented Aug 4, 2017 • edited Loading

coveralls commented Aug 4, 2017

coveralls commented Aug 4, 2017

coveralls commented Aug 4, 2017

kwonoj commented Aug 4, 2017

hermanbanken commented Aug 5, 2017

hermanbanken commented Aug 7, 2017 • edited Loading

coveralls commented Aug 7, 2017

kwonoj commented Aug 7, 2017

hermanbanken commented Aug 7, 2017

coveralls commented Aug 7, 2017

coveralls commented Aug 7, 2017 • edited Loading

benlesh commented Mar 30, 2018

cartant commented May 30, 2018 • edited Loading

benlesh left a comment

Choose a reason for hiding this comment

benlesh May 30, 2018

Choose a reason for hiding this comment

benlesh May 30, 2018

Choose a reason for hiding this comment

benlesh May 30, 2018

Choose a reason for hiding this comment

benlesh May 30, 2018

Choose a reason for hiding this comment

benlesh May 30, 2018

Choose a reason for hiding this comment

benlesh May 30, 2018

Choose a reason for hiding this comment

cartant commented May 30, 2018 • edited Loading

hermanbanken commented May 31, 2018

cartant commented May 31, 2018

cartant commented May 31, 2018

benlesh commented May 31, 2018

hermanbanken commented Aug 4, 2017 •

edited

Loading

rxjs-bot commented Aug 4, 2017 •

edited

Loading

hermanbanken commented Aug 7, 2017 •

edited

Loading

coveralls commented Aug 7, 2017 •

edited

Loading

cartant commented May 30, 2018 •

edited

Loading

cartant commented May 30, 2018 •

edited

Loading