Opened 7 years ago

Closed 7 years ago

#15713 closed defect (fixed)

[regression] dojo/_base/Deferred silently discards errors thrown in callback

Reported by: Colin Snover Owned by: Colin Snover
Priority: blocker Milestone: 1.8
Component: General Version: 1.8.0b1
Keywords: Cc: bill, Kitson Kelly
Blocked By: Blocking:

Description (last modified by Colin Snover)

Test case: http://jsfiddle.net/TZaZF/1/

Expected: Error logged to console (like 1.7)

Actual: Silently fails

Change History (18)

comment:1 Changed 7 years ago by Colin Snover

Description: modified (diff)

comment:2 Changed 7 years ago by bill

Note that 1.7 would log the errors to the console. This is probably just a manifestion of your conservative instrumentation, that only triggers when dojo/main is loaded. My instinct is that it should trigger whenever isDebug:true.

comment:3 Changed 7 years ago by Kitson Kelly

I have tried doing:

require(["dojo/_base/Deferred", "dojo/promise/instrumenting"], ...);
// and
require(["dojo/_base/Deferred", "dojo/main"], ...);
// and
require(["dojo/_base/Deferred", "dojo/main", "dojo/promise/instrumenting"], ...);

And none of them cause the error to be thrown.

I did the following: http://jsfiddle.net/kitsonk/TZaZF/3/ and that causes the error to be logged to the console, but rethrowing the error gets swallowed, which isn't good.

Moving back to 1.7.2 I get two error thrown in my example above.

comment:4 Changed 7 years ago by cjolif

what are you running guys? the beta1 or trunk? Because in trunk (not built) I _do_ get an error in the console? (without doing anything special).

Error
 "Error: Oops
    at http://localhost/dojo-trunk-commit/dojo/tests/test_onkeydown.html:11:19
    at notify (../_base/Deferred.js:187:23)
    at complete (../_base/Deferred.js:168:4)
    at [object Object].<anonymous> (../_base/Deferred.js:215:4)
    at http://localhost/dojo-trunk-commit/dojo/tests/test_onkeydown.html:14:13
    at http://localhost/dojo-trunk-commit/dojo/dojo.js:1071:43
    at http://localhost/dojo-trunk-commit/dojo/dojo.js:1200:5
    at http://localhost/dojo-trunk-commit/dojo/dojo.js:770:6
    at http://localhost/dojo-trunk-commit/dojo/dojo.js:124:11
    at http://localhost/dojo-trunk-commit/dojo/tests/test_onkeydown.html:7:5" ../promise/instrumenting.js:22
Last edited 7 years ago by cjolif (previous) (diff)

comment:5 Changed 7 years ago by Kitson Kelly

@cjolif look at the JSFiddles, they come from nightly... It could be XD related though... I will try tests in my local sandbox, but the jsFiddles are pretty straight forward. As soon as you drop back to 1.7.2, the behaviour comes back.

Also, it appears that this isn't specific to Deferred.when() either... basically any old callback is swallowing errors: http://jsfiddle.net/kitsonk/ceNDP/

And even new style Deferreds: http://jsfiddle.net/kitsonk/yx6r6/

I went and checked the dojo/parser, because I specifically put tests in there to actually ensure error get thrown and not not swallowed and I am still seeing those, so I am not sure what is actually happening.

comment:6 Changed 7 years ago by cjolif

They get their nightly from: http://archive.dojotoolkit.org/nightly/dojotoolkit/

  1. it has not been updated since July 17 (but I don't think this is the actual problem)
  2. this is a built version. In built version the instrumentation is stripped out by default

If I do a local built I reproduce the "issue". If I override the build settings to do something like:

'config-useDeferredInstrumentation':1, 'config-deferredInstrumentation':"report-unhandled-rejections",

then it is working with build version as well.

So I think the real problem is "just" that built version does not include error reporting by default. Maybe we should just revert that default to get back to 1.7 behavior?

Last edited 7 years ago by cjolif (previous) (diff)

comment:7 Changed 7 years ago by Mark Wubben

Could you summarize for me what the issue is?

So I think the real problem is "just" that built version does not include error reporting by default. Maybe we should just revert that default to get back to 1.7 behavior?

There's a fair amount of overhead in the instrumenting, which is why it shouldn't be enabled by default in built versions.

comment:8 Changed 7 years ago by cjolif

If I'm not mistaken the issue is in the fiddle it the summary. Running that piece of code against a built dojo version was printing console errors in 1.7 and it does not anymore. Colin considers this as a blocker.

comment:9 Changed 7 years ago by Kitson Kelly

Plus I can't seem to get it turned on in the built version: http://jsfiddle.net/kitsonk/wsdwq/

I think I am doing everything right, but no matter what I try, I can't get it to throw, but if you drop back to 1.7.2 it works fine. There are significant amount of people that would do development off a CDN, right or wrong.

comment:10 Changed 7 years ago by cjolif

For the log: we discussed on IRC about a solution that would be to enable instrumentation / error reporting for promise/Deferred when has("dojo-debug-messages") (potentially triggered by isDebug) is enabled irrespective to the fact this is a build version or not.

Last edited 7 years ago by cjolif (previous) (diff)

comment:11 Changed 7 years ago by Colin Snover

Using the given test case running unbuilt against current trunk (r29333) this problem continues to exist as originally described. cjolif I don’t know how you are experiencing anything different but note you are apparently running some onkeydown test which is not the given test case?

comment:12 Changed 7 years ago by cjolif

csnover, well I pasted your code in this test case just for convenience. But don't worry this is your test case that I run and I confirm I have no problem on unbuilt versions. markwubben has a patch ready that fixes that as well in built versions (provided dojo-debug-messages in on) as Bill mentioned on the mailing list.

Last edited 7 years ago by cjolif (previous) (diff)

comment:13 Changed 7 years ago by Colin Snover

That test case doesn’t seem to be one that exists in any version of Dojo I have access to so I can’t confirm how it is written. However I can pretty much guarantee that it does not match the test case I provided. It sounds like it loads dojo/main, which is *not* the same as the provided test case, and would result in the behaviour you are describing (i.e. errors presenting in trunk).

comment:14 Changed 7 years ago by Mark Wubben

In [29335]:

Fix to old Deferred instrumentation

Refs #15713 !strict

comment:15 Changed 7 years ago by Mark Wubben

In [29336]:

Updates to how Deferred instrumentation is started

  • Change deferredInstrumentation config to control whether the instrumentation is included
  • Change useDeferredInstrumentation config to control whether the instrumentation is used
  • Removes dojo/promise/instrumenting and makes it private under dojo/promise/instrumentation.

useDeferredInstrumentation can be 1 and true, which makes it default to report-unhandled-rejections, or report-rejections.

During development useDeferredInstrumentation defaults to report-unhandled-rejections. It's enabled whenever dojo/Deferred is included.

Refs #15713 !strict

comment:16 Changed 7 years ago by Mark Wubben

In [29337]:

Change default build to include deferred instrumentation infrastructure, but leave it unused.

Refs #15713 !strict

comment:17 Changed 7 years ago by Mark Wubben

Owner: changed from Mark Wubben to Colin Snover
Status: newassigned

All landed. Colin perhaps you could verify with your local tests as well?

comment:18 Changed 7 years ago by Colin Snover

Resolution: fixed
Status: assignedclosed

Verified.

Note: See TracTickets for help on using tickets.