Repository navigation
doc: more use-cases for promise events #979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
cdfa271
7ee58be
3bc7e58
278a926
b8341e8
154d3f5
c7be08c
31450fc
22097a2
297b9ae
8f87169
380a3d8
f8152df
f442904
81a0c0b
d2f70fe
a6d674d
acb6779
e1fb0e8
a1949e8
958a94e
68dc69a
8e14135
de05175
bcc6d47
a787f72
abb2a4b
892bf65
999d25b
414e407
5579e85
7f5d6c9
a85f4b5
fed0b28
40ec84d
05d30d5
1f842c2
3bba619
3351305
7ec0491
c70c778
91e7e9c
467eff2
845acb4
b981364
52f84ad
704dcce
2a17c7f
360d0fa
942da88
7d927ab
e7a3ca3
7d79412
74178a5
16f86d6
94e663a
a12628b
e148dcc
6cb9e8a
3b78b85
71c53b6
43cb1dd
9aa6a43
5f6579d
2b8a06b
d7da617
e4f2952
1cfee8e
2853f98
1fa0cb8
870229e
921f2de
4dcf24c
e0c3d2a
0a329d2
e29e470
4144cb0
a9ad31f
e68a119
4fb4c14
229a03f
61d48b4
57c4432
2c09a53
4e028bd
571a517
5819d7a
451acc7
75fe773
fcfd87d
79d2c4e
ccea33d
7953c83
f32a606
4a6f1fe
5ac5419
ffdbe9d
d0d4877
28da400
2d21092
76c80d4
79ebeab
02448c6
c22bcd3
6ac79bc
36b969f
6df47d6
af881ad
1b78151
b50e89e
6192c98
590ee58
212c9c0
24fe112
680dda8
44efd66
306936e
20dae2a
30b8bb0
f1d27d2
0e4b772
dd8b267
3f62c40
9a1bc4e
dbe4844
c273944
d32363f
a6aab43
07a43eb
e83499a
e52864b
3f476ad
6be2a59
9bd26e7
02fe821
87e820e
5bbc6df
2e6ece4
6fff47f
342c3a1
0504066
b3d9678
a4fa51c
820735d
59cb053
8593b3e
49dec1a
77a10ed
089d688
8853388
6ad458b
070aac4
81e98e9
7d66749
d25e34b
aa97ae7
87d2615
a1bda1b
8a685e7
05d424c
0094a8d
e655a43
dbce29a
c78091d
3de353b
d1f2404
184f09d
20b68e5
66c9c61
f5a3f44
6f63a4a
0116da1
10924ce
a334ddc
099cfbc
17665af
07a8419
aeee956
a18dd7b
35cdded
a214905
a9d42e0
ccaaae9
ded4f91
178ac33
8aa589c
01908d0
47befff
a1040f2
72c8a74
70c78c9
2146f62
048c8f2
775c01e
e97dae5
ead5cd9
aff3acf
9148114
b9279aa
31c971d
0474bf5
3ef2e4a
8e21309
6899094
3202456
a713024
51325c0
47db247
bde32f8
7d94611
931118c
6f14b3a
ff877e9
4bc9d97
0a7076b
2d35607
e3d9d25
81503e5
85b74de
f45c315
ab03635
6c032a8
a441aa6
d1a2e53
d8011d1
7fb128d
4fc6388
bbdbef9
5cd1fd8
1fdec65
972a0c8
770cd22
8854183
dae9fae
503f279
6d92eba
f72e178
0140e1b
676e618
b52142f
642928b
419f7d4
fc72a57
8c043c1
0f99320
0803962
d8db757
286ef1d
73b7e05
d39c1cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
This adds an example of a semi-common promise pattern that could result in false-positive 'unhandledRejection' events, to help motivate why there is a dual 'rejectionHandled' event. I anticipate it being helpful to users who encounter 'unhandledRejection' events that do not stem from obvious typos such as the JSON.pasre example. Also cleans up the promise rejection tracking sample. By using a Set instead of array we can get O(1) deletion. And, fixed indentation and backtick usage there to align with the rest of the document.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,40 +159,60 @@ event: | |
| return reportToUser(JSON.pasre(res)); // note the typo | ||
| }); // no `.catch` or `.then` | ||
|
|
||
| Here is an example of a coding pattern that will also trigger | ||
| `'unhandledRejection'`: | ||
|
|
||
| function SomeResource() { | ||
| // Initially set the loaded status to a rejected promise | ||
| this.loaded = Promise.reject(new Error('Resource not yet loaded!')); | ||
| } | ||
|
|
||
| var resource = new SomeResource(); | ||
| // no .catch or .then on resource.loaded for at least a turn | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker for this PR, but it'd be nice to standardize / document our language around "turn of the event loop" / "next tick."
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "turn" is used here on purpose, it's not a "next tick" in the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah – Promises execute out of the microtask queue, if I'm not mistaken, which are related to the execution of nextTick, which interleaves microtask runs with nextTick callbacks scheduled inside nextTicks... so we get into a foggy area with regards to what a "turn" represents. The thrust of it is clear, but the specifics get hairy; it might be good to document what we mean when we use the general term in terms of the specific actions someplace. In any case, I might have just talked myself into writing a PR for timers.markdown (with apologies to @trevnorris). :) edit: and my above summary is still slightly incorrect! It goes "tick", ("nextTick", "microtask" )*, I think.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I think "turn" is good terminology here is that most developers don't know what a microtask or a macrotask is but understand "event loop turn". As the person who used this terminology in the original docs PR I think "turn" is the way to go here - that said a clarification page in the documentation on all that would be nice.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "until next tick" would be more accurate
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vkurchatkin more accurate, but much less approachable to users who don't really understand the order the even loop is spun in io.js. Just two months ago I wasn't too aware of it myself and I've been using node for over three years at that point and have used multiple concurrency libraries and promises for at least a year and a half of that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a valid issue, but not reason to give inaccurate information. in fact "ticks" have nothing do with event loop at all
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point. Maybe inaccurate but approachable is better than unapproachable? That said if you think there is better terminology for this that developers will understand I'm all for it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree. In this case it's really closer to "wrong" than "inaccurate".
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the solution is to create a document on io's async model that explains what a microtask is, a macrotask is and a queue of tasks is. Otherwise I really can't see how we can expect people to understand this terminology. |
||
|
|
||
| To deal with cases like this, which you likely don't want to track as developer | ||
| error in the same way as other `'unhandledRejection'` events, you can either | ||
| attach a dummy `.catch(function() { })` handler to `resource.loaded`, | ||
| preventing the `'unhandledRejection'` event from being emitted, or you can use | ||
| the 'rejectionHandled' event. | ||
|
|
||
| ## Event: 'rejectionHandled' | ||
|
|
||
| Emitted whenever a Promise was rejected and an error handler was attached to it | ||
| (for example with `.catch()`) later than after an event loop turn. This event | ||
| is emitted with the following arguments: | ||
|
|
||
| - `p` the promise that was previously emitted in an 'unhandledRejection' | ||
| - `p` the promise that was previously emitted in an `'unhandledRejection'` | ||
| event, but which has now gained a rejection handler. | ||
|
|
||
| There is no notion of a top level for a promise chain at which rejections can | ||
| always be handled. Being inherently asynchronous in nature, a promise rejection | ||
| can be be handled at a future point in time — possibly much later than the | ||
| event loop turn it takes for the 'unhandledRejection' event to be emitted. | ||
| event loop turn it takes for the `'unhandledRejection'` event to be emitted. | ||
|
|
||
| Another way of stating this is that, unlike in synchronous code where there is | ||
| an ever-growing list of unhandled exceptions, with promises there is a | ||
| growing-and-shrinking list of unhandled rejections. In synchronous code, the | ||
| 'uncaughtException' event tells you when the list of unhandled exceptions | ||
| grows. And in asynchronous code, the 'unhandledRejection' event tells you | ||
| grows. And in asynchronous code, the `'unhandledRejection'` event tells you | ||
| when the list of unhandled rejections grows, while the 'rejectionHandled' | ||
| event tells you when the list of unhandled rejections shrinks. | ||
|
|
||
| For example using the rejection detection hooks in order to keep a list of all | ||
| the rejected promises at a given time: | ||
|
|
||
| var unhandledRejections = []; | ||
| var unhandledRejections = new Set(); | ||
| process.on('unhandledRejection', function(reason, p) { | ||
| unhandledRejections.push(p); | ||
| unhandledRejections.add(p); | ||
| }); | ||
| process.on('rejectionHandled', function(p) { | ||
| var index = unhandledRejections.indexOf(p); | ||
| unhandledRejections.splice(index, 1); | ||
| unhandledRejections.delete(p); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if it rejects the same promise twice? (Is that even possible?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not possible :) |
||
| }); | ||
|
|
||
| You could then record this list in some error log, either periodically or upon | ||
| process exit. This would avoid the false positive seen in the above resource | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "avoiding false positives" is misleading, this doesn't eliminate false positives, in both cases of logging periodically/on process exit there can be false positives.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @petkaantonov the periodically comment is spot on - although if it's on process exit there can't be a false positive.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @benjamingr why not? Process can exit for many other reasons than there not being anything to be done. For instance when an error is thrown and not caught.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's misleading but it's not a false positive. Since the process has terminated nothing else can handle the rejection so it's just a positive. It's still problematic in that it's not useful in this case.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not a useful definition for false positive. This code path doesn't have any bugs regarding to not handling errors, and it would cause the server logs to be filled with false positives regardless if you use periodic logging or logging-on-exit: function doIt(request) {
var a = Promise.reject(new Error());
setTimeout(function() {
a.catch(function(e) {
request.sendError(e.toString());
});
}, 10);
}
server.onRequest(doIt);
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean sure, that specific instance of rejection will technically not be handled by the code since the process was exited but the whole point is to surface the programmer errors of not handling errors in a some code paths. In that, much more useful sense, logging-on-exit will cause false positives.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That last comment clarifies it - I didn't understand you were talking about code paths and not specific rejections.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This isn't a panacea for all false positives. This is an example of one solution for the example given, meant to illustrate the purpose of the 'rejectionHandled' event.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand how this avoids the "false positive" error. By not crashing when there is an error?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By not logging errors that are not developer errors, but are instead a system being in an errored state but eventually transitioning out of it. |
||
| example. | ||
|
|
||
| ## Signal Events | ||
|
|
||
| <!--type=event--> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the debate actually around whether or not this is a good pattern?
At second look this looks unusual to me, I would expect it to wait until there is something or a timeout, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's perhaps part of it. But this pattern of using promises to signal state transitions is reasonably common; see http://www.w3.org/2001/tag/doc/promises-guide#state-transitions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very unusual to me too. I tend to agree, especially since people who subscribe to
loadedshould be able to get a hook on when it happened.Better yet - the construction should not pretend to perform IO.
I do think that we can find an example of an operational (not programmer) error happening with promises without it to document something that causes
unhandledRejectionand requires adding a.catch- for example reporting a non-critical event is pretty common, or sending an email in the background and not caring if it succeeded.