doc: more use-cases for promise events #3438

domenic · 2015-10-19T17:04:56Z

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.

Supercedes #979. Hopefully things can be more civil this time.

benjamingr · 2015-10-19T17:05:39Z

Please consider the discussion in #979 before merging.

If we warn about the two issues in the PR (that the Set can leak, and that you still have false positives) I'm more OK with it (although I definitely think we should wait for @petkaantonov to weigh in here).

and #979 (comment)

benjamingr · 2015-10-19T17:08:52Z

Basically, summing up the pending issues I have with it:

It should warn about the solution not working for all false positives and that there are still false positives (it is an inherent problem with a temporal construct that changes states).
It should warn about the potential memory leak.
The constructor example should probably be clarified (as @Fishrock123 noted). As in the discussion doc: more use-cases for promise events #979 (comment) .

When these gets addressed I'm all for merging.

Fishrock123 · 2015-10-19T17:10:24Z

doc/api/process.markdown

+
+    function SomeResource() {
+      // Initially set the loaded status to a rejected promise
+      this.loaded = Promise.reject(new Error('Resource not yet loaded!'));


This patten is confusing to me. How does data actually get there once loaded?

I have not shown a complete implementation of SomeResource. I could do so if it would help. Essentially there's a SomeResource.prototype.load() method that will replace the promise with a successful one.

The idea is that anyone doing .loaded.then(...) before anything is loaded goes down the error path. Whereas, after something is loaded, they go down the success path.

I fleshed this out in full painful detail https://gist.github.com/domenic/854f0dc0f698a63b0e8a. I think including all that would be way too much for the docs, but if there's a way to make it clearer, I'm happy to do so.

That makes sense.

Qard · 2015-10-19T17:12:37Z

I'm fine with merging this, if it better highlights the potentially leaky aspect of the unhandledRejections set and the fact that, at any given time, not all items in unhandledRejections are necessarily in a state that they would never be handled.

As for my comment on making it a topic doc, I don't think that should be a blocker, but should be something we think about in the future. As yet, the non-reference docs situation is not yet entirely nailed down.

Fishrock123 · 2015-10-19T17:12:50Z

doc/api/process.markdown

    });

+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


Maybe better: "This would then only expose the developer errors as shown above in'unhandledRejection' example."

Sure, since some people are allergic to the term false positive that seems like a good way to defuse the situation.

You could then record this list in some error log, either periodically or upon process exit. This would avoid the false positive

This doesn't avoid the false positive. It makes a false positive significantly less likely. I don't think those are the same thing, but if there is otherwise consensus then I won't object it.

Used @Fishrock123's phrasing

domenic · 2015-10-19T17:16:52Z

I'm fine with merging this, if it better highlights the potentially leaky aspect of the unhandledRejections set

Can you suggest some phrasing? All I can think of is "since the unhandledRejection set lives for the lifetime of your program, it will consume memory for the lifetime of your program, the same as every other object that lives for the lifetime of your program."

domenic · 2015-10-19T17:20:17Z

the fact that, at any given time, not all items in unhandledRejections are necessarily in a state that they would never be handled.

I tried to cover this in the existing text with "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"

petkaantonov · 2015-10-19T17:33:58Z

Can you suggest some phrasing? All I can think of is "since the unhandledRejection set lives for the lifetime of your program, it will consume memory for the lifetime of your program, the same as every other object that lives for the lifetime of your program."

I don't think it's very good to make an example that is essentially a memory leak and then basically say "btw this obviously leaks memory, figure out some way to avoid that". There is just no way to phrase that well. The example should be fixed so that there is no inherent memory leak to begin with e.g. some interval that periodically clears the list or such.

Fishrock123 · 2015-10-19T17:35:14Z

doc/api/process.markdown

    });

+You could then record this list in some error log, either periodically or upon
+process exit. This would then only expose the developer errors as shown above
+in the `'unhandledRejection'` example.


Perhaps it is better to reword and put this into the 'unhandledRejection' part, it's missing context here.

"below is an example ..."

I rephrased it, hopefully in a way that addresses @petkaantonov's concerns as well.

Fishrock123 · 2015-10-19T17:42:28Z

I'm not 100% sure about the entire pre-populate error-promise as a notification thing, but the wording of the rest of the docs now appears sound. I'll let others chime in.

Fishrock123 · 2015-10-19T17:43:35Z

doc/api/process.markdown

+    // no .catch or .then on resource.loaded for at least a turn
+
+To deal with cases like this, which you may not want to track as developer
+error in the same way as other `'unhandledRejection'` events, you can either


"which you may not want to track as developer error in the same way as other 'unhandledRejection' events"

Sentence structure is a little weird here, also "errors".

Split into two sentences and rephrased a bit.

Qard · 2015-10-19T17:50:14Z

@domenic I think the wording needs to make more reference to the content of the set, not the existence of the set. It will continue to be a single set through the life of the program, but the content could grow over time. In an app that is not sufficiently handling promises, it could grow to the point of consuming all memory and terminating the app.

I'm not sure I agree exactly with @petkaantonov on the need for code to clear the list, just due to the verbosity of that, but at least explaining that real code would need to do that is important, I think.

domenic · 2015-10-19T18:01:28Z

@Qard at the cost of some extra verbosity, expanded the last paragraph along those lines.

Qard · 2015-10-19T18:06:12Z

LGTM

petkaantonov · 2015-10-19T18:08:23Z

lgtm

benjamingr · 2015-10-19T18:08:41Z

This is much better - what about the constructor example? I'd be happy if we changed that a bit.

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 Map instead of array we can get O(1) deletion and actually record the errors. And, fixed indentation and backtick usage there to align with the rest of the document.

benjamingr · 2015-10-20T12:52:46Z

I saw an interesting use case today, what about something like the following?

    var promises = [];
    myEventEmitter.on("data", url => {
          promises.push(makeRequest(url));
    }).on("done", () => Promise.all(promises).then(results => {
           // do work
    });

Without suppressing the error makeRequest might cause - this will cause an unhandled rejection most likely, and creating promises inside an event emitter seems pretty common. I think it might be a good alternative to the current example with the constructor.

Again, other than that LGTM.

domenic · 2015-10-20T16:02:33Z

This has gotten LGTMs from five collaborators now (1, 2, 3, 4, 5). There is one vocal non-collaborator who wants to make further changes, but I respectfully ask that we merge this now and @benjamingr can make any further changes as pull requests.

benjamingr · 2015-10-20T17:06:30Z

To clarify - as I said in my last two comments - it looks good to me, further improvements can go to a new PRs.

The tone I do not appreciate so much but I can live with it :)

jasnell · 2015-10-21T17:36:38Z

LGTM. Will land.

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 Map instead of array we can get O(1) deletion and actually record the errors. And, fixed indentation and backtick usage there to align with the rest of the document. Reviewed-By: Stephan Belanger <[email protected]> Reviewed-By: Petka Antonov <[email protected]> Reviewed-By: James M Snell <[email protected]> PR-URL: #3438

jasnell · 2015-10-21T17:40:32Z

Landed in d381290

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 Map instead of array we can get O(1) deletion and actually record the errors. And, fixed indentation and backtick usage there to align with the rest of the document. Reviewed-By: Stephan Belanger <[email protected]> Reviewed-By: Petka Antonov <[email protected]> Reviewed-By: James M Snell <[email protected]> PR-URL: #3438

MylesBorins · 2015-10-23T18:55:21Z

should this be in lts?

@jasnell

jasnell · 2015-10-23T19:34:24Z

Could be but not a pressing need.
On Oct 23, 2015 11:55 AM, "Myles Borins" [email protected] wrote:

should this be in lts?

@jasnell https://github.com/jasnell

—
Reply to this email directly or view it on GitHub
#3438 (comment).

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 Map instead of array we can get O(1) deletion and actually record the errors. And, fixed indentation and backtick usage there to align with the rest of the document. Reviewed-By: Stephan Belanger <[email protected]> Reviewed-By: Petka Antonov <[email protected]> Reviewed-By: James M Snell <[email protected]> PR-URL: #3438

jasnell · 2015-10-30T16:38:03Z

Landed in v4.x-staging in bb73029

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 Map instead of array we can get O(1) deletion and actually record the errors. And, fixed indentation and backtick usage there to align with the rest of the document. Reviewed-By: Stephan Belanger <[email protected]> Reviewed-By: Petka Antonov <[email protected]> Reviewed-By: James M Snell <[email protected]> PR-URL: #3438

domenic added the doc Issues and PRs related to the documentations. label Oct 19, 2015

Fishrock123 reviewed Oct 19, 2015
View reviewed changes

domenic force-pushed the extended-promise-examples branch from d39c1cf to 07d7a2b Compare October 19, 2015 17:20

Fishrock123 reviewed Oct 19, 2015
View reviewed changes

domenic force-pushed the extended-promise-examples branch from 07d7a2b to 6489d4a Compare October 19, 2015 17:39

Fishrock123 reviewed Oct 19, 2015
View reviewed changes

domenic force-pushed the extended-promise-examples branch from 6489d4a to fd50bcd Compare October 19, 2015 17:47

mscdex added the process Issues and PRs related to the process subsystem. label Oct 19, 2015

domenic force-pushed the extended-promise-examples branch from fd50bcd to 9e98329 Compare October 19, 2015 18:01

domenic force-pushed the extended-promise-examples branch from 9e98329 to 6047052 Compare October 19, 2015 18:10

jasnell closed this Oct 21, 2015

This was referenced Oct 27, 2015

Release proposal: v5.0.0 #3466

Merged

Preparing v4.2.2 LTS Maintenance nodejs/Release#50

Closed

Release Proposal: v4.2.2 "Argon" (LTS) #3588

Closed

jasnell added the lts-watch-v4.x label Oct 30, 2015

jasnell added land-on-v4.x and removed lts-watch-v4.x labels Oct 30, 2015

jasnell removed the land-on-v4.x label Nov 3, 2015

leonardoadame mentioned this pull request Nov 28, 2023

[Snyk] Fix for 6 vulnerabilities leonardoadame/node#185

Open

aliscco mentioned this pull request Nov 28, 2023

[Snyk] Fix for 5 vulnerabilities aliscco/alisco-node#694

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 6 vulnerabilities leonardoadame/node#210

Open

This was referenced Nov 29, 2023

[Snyk] Fix for 4 vulnerabilities aliscco/alisco-node#714

Open

[Snyk] Fix for 5 vulnerabilities aliscco/alisco-node#732

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 5 vulnerabilities leonardoadame/node#263

Open

This was referenced Nov 29, 2023

[Snyk] Fix for 14 vulnerabilities aliscco/alisco-node#776

Open

[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#1023

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: more use-cases for promise events #3438

doc: more use-cases for promise events #3438

domenic commented Oct 19, 2015

benjamingr commented Oct 19, 2015

benjamingr commented Oct 19, 2015

Fishrock123 Oct 19, 2015

domenic Oct 19, 2015

domenic Oct 19, 2015

Fishrock123 Oct 19, 2015

Qard commented Oct 19, 2015

Fishrock123 Oct 19, 2015

domenic Oct 19, 2015

petkaantonov Oct 19, 2015

domenic Oct 19, 2015

domenic commented Oct 19, 2015

domenic commented Oct 19, 2015

petkaantonov commented Oct 19, 2015

Fishrock123 Oct 19, 2015

domenic Oct 19, 2015

Fishrock123 commented Oct 19, 2015

Fishrock123 Oct 19, 2015

domenic Oct 19, 2015

Qard commented Oct 19, 2015

domenic commented Oct 19, 2015

Qard commented Oct 19, 2015

petkaantonov commented Oct 19, 2015

benjamingr commented Oct 19, 2015

benjamingr commented Oct 20, 2015

domenic commented Oct 20, 2015

benjamingr commented Oct 20, 2015

jasnell commented Oct 21, 2015

jasnell commented Oct 21, 2015

MylesBorins commented Oct 23, 2015

jasnell commented Oct 23, 2015

jasnell commented Oct 30, 2015

doc: more use-cases for promise events #3438

doc: more use-cases for promise events #3438

Conversation

domenic commented Oct 19, 2015

benjamingr commented Oct 19, 2015

benjamingr commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qard commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Oct 19, 2015

domenic commented Oct 19, 2015

petkaantonov commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qard commented Oct 19, 2015

domenic commented Oct 19, 2015

Qard commented Oct 19, 2015

petkaantonov commented Oct 19, 2015

benjamingr commented Oct 19, 2015

benjamingr commented Oct 20, 2015

domenic commented Oct 20, 2015

benjamingr commented Oct 20, 2015

jasnell commented Oct 21, 2015

jasnell commented Oct 21, 2015

MylesBorins commented Oct 23, 2015

jasnell commented Oct 23, 2015

jasnell commented Oct 30, 2015