Deferred: Warn on exceptions that are likely programming errors #2737

dmethvin · 2015-11-23T19:13:56Z

Still a WIP with some details to be worked out. I'm not really happy with the stack trace that results, it's only showing the immediate prior call. The gist, however, is that nearly all exceptions of these types are programming mistakes and should be reported ASAP. If devs don't like being warned about them they can set jQuery.Deferred.exceptionHook = null and not be bothered, or define a custom hook that filters out the noisy false positives.

timmywil · 2015-11-23T19:59:46Z

Yea, I wonder if error.stack is a more appropriate stack trace than console.trace().

gibson042 · 2015-11-23T20:10:21Z

src/deferred.js

I still don't see this as library code. The whole mess is more appropriate for userland.

I'd agree if the developer ergonomics weren't so horrific. If the default is to swallow but we just add the ability for a hook, people won't know enough to add a hook. Like I said in the meeting, devs need a solution really bad, so here is a really bad solution.

What about pluging in this "error-handler" via a separate AMD Module...? That way one can build jQuery without it (for production) and with it for development etc

@staabm not a bad idea

scottgonzalez · 2015-11-23T21:21:26Z

Is my understanding correct that this:

Warns on all exceptions, not just unhandled exceptions
Only listens for thrown exceptions and not explicitly rejected promises?

dmethvin · 2015-11-23T21:26:16Z

Warns on all exceptions, not just unhandled exceptions

No, it only warns on the derivative Error types listed there.

Only listens for thrown exceptions and not explicitly rejected promises?

Yes, non-exception rejections (not an exception or explicitly thrown error) don't go through this path. jQuery Deferred hasn't ever warned on unhandled explicit rejections (such as an ajax 404) so I don't feel bad about excluding them.

scottgonzalez · 2015-11-24T13:47:58Z

Warns on all exceptions, not just unhandled exceptions

No, it only warns on the derivative Error types listed there.

What I mean is that this isn't a hook for unhandled exceptions. It's a hook for all exceptions.

dmethvin · 2015-11-24T13:52:14Z

What I mean is that this isn't a hook for unhandled exceptions. It's a hook for all exceptions.

Oh, sorry, yes. Exceptions like SyntaxError and explicitly thrown errors will pass through here.

timmywil · 2015-11-25T17:21:47Z

src/deferred.js

Could we do error.stack here?

dmethvin · 2015-12-01T00:38:00Z

Pushed a few changes here. I tried sinon.spy but was getting an error from its guts and didn't really need it anyway.

dmethvin · 2015-12-01T04:57:49Z

src/deferred/exceptionHook.js

Because the Deferred is resolved through a setTimeout it loses the stack. We could use some chicanery to save a stack beforehand but that would impact size and performance for the comparatively rare case where an exception occurs. So I pulled out the stack trace entirely since it only refers to things inside jQuery itself.

Does simply accessing the stack property of an Error have a perf impact? You suggested this in the meeting, but you could also provide jquery.longStackTraces.js as a plugin or something to get functionality similar to Bluebird as long as the appropriate hooks exist in core

Adding a hook so that a plugin could provide better stack traces seems like a good way to go, I'm looking into it.

Fixes jquerygh-2736

* The stack is worthless because the promise is resolved by setTimeout() * New module created for exceptionHook() * The .stack property is not useful for our purposes

dmethvin · 2015-12-20T20:40:51Z

Updated to add the stack hook. I'm not fully confident about what's here, would love a review, esp from @gibson042.

dmethvin · 2015-12-20T20:42:05Z

test/unit/deferred.js

This needs to be removed but I wanted to be sure we had the valid stack at this point of execution.

dcherman · 2015-12-23T17:59:43Z

src/deferred/exceptionHook.js

Not that this matters in the slightest, but seeing as how you're accessing the global immediately above this as feature detection, is there any reason to .call rather than just window.console.warn(...)? Might even save some bytes ;)

Yeah, technically I could just let gzip do its thang and avoid the variable, which would also remove the need for the .call. I'm okay with that if people prefer it.

Sounds like a good idea.

dmethvin · 2015-12-28T22:21:04Z

I've put together a plugin that uses the hook for collecting the stack and then shows it when an exception occurs. In the README I mention that having to collect the stack in advance makes Deferreds slower by roughly 2x which is why it isn't on by default. However, since it's such a small amount of code I wonder whether we shouldn't just have a flag or other simple one-liner to enable this rather than making people load another module/plugin to do it.

timmywil · 2015-12-28T23:07:36Z

I like that it's a separate plugin, with its own README explaining why it's necessary. A flag can easily be kept on by accident.

jquerybot added the CLA: Valid label Nov 23, 2015

gibson042 reviewed Nov 23, 2015
View reviewed changes

timmywil reviewed Nov 25, 2015
View reviewed changes

src/deferred.js Outdated

Copy link

Member

timmywil Nov 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do error.stack here?

dmethvin reviewed Dec 1, 2015
View reviewed changes

dmethvin added 5 commits December 15, 2015 16:59

Deferred: Warn on exceptions that are likely programming errors

398e543

Fixes jquerygh-2736

[SQUASH]: Updates per code review suggestions

8f60f04

* The stack is worthless because the promise is resolved by setTimeout() * New module created for exceptionHook() * The .stack property is not useful for our purposes

[SQUASH]: Improve unit test

2a6671d

[SQUASH]: Don't use .catch()

54391c4

[SQUASH]: Add hook for getting stack info before async call

79d7c6b

dmethvin force-pushed the 2736-unhandled-exceptions branch from c311290 to 79d7c6b Compare December 20, 2015 20:39

dmethvin reviewed Dec 20, 2015
View reviewed changes

[SQUASH]: Use passed-in stack from pre-setTimeout era

de90f0a

dcherman reviewed Dec 23, 2015
View reviewed changes

dmethvin mentioned this pull request Dec 29, 2015

Do you actually need to throw? dmethvin/jquery-deferred-reporter#2

Closed

[SQUASH]: Call window.console.warn directly

2a5ce15

dmethvin closed this in 36a7cf9 Jan 13, 2016

dmethvin deleted the 2736-unhandled-exceptions branch May 24, 2016 19:08

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Deferred: Warn on exceptions that are likely programming errors #2737

Deferred: Warn on exceptions that are likely programming errors #2737

Uh oh!

Conversation

dmethvin commented Nov 23, 2015

Uh oh!

timmywil commented Nov 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottgonzalez commented Nov 23, 2015

Uh oh!

dmethvin commented Nov 23, 2015

Uh oh!

scottgonzalez commented Nov 24, 2015

Uh oh!

dmethvin commented Nov 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmethvin commented Dec 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmethvin commented Dec 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol Dec 23, 2015 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmethvin commented Dec 28, 2015

Uh oh!

timmywil commented Dec 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

10 participants

mgol Dec 23, 2015 •

edited

Loading