Skip to content
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

doc: add unhandled promise rejection survey #857

Closed
wants to merge 15 commits into from

Conversation

mmarchini
Copy link
Contributor

Since we decided to have a survey before voting on nodejs/node#33021, I started to draft the survey to capture community feedback. This is by no means the final version of the survey, it's just a starting point as I believe we should build this survey together.

Please leave comments suggestion questions we should include or remove, or any changes to the existing questions from title to description to available options.

surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
@mmarchini
Copy link
Contributor Author

I think I addressed all comments. I turned the first two questions into multiple-choice questions (still requiring choosing at least one answer), broke the global handler question into two Yes/No questions, and added the memory leak question as the last one. Let me know if y'all have any other suggestions.

surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
- [ ] `none`
- [ ] Other (please elaborate)

# Are you aware that not handling rejections may cause memory leaks or denial of service vulnerabilities?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Are you aware that not handling rejections may cause memory leaks or denial of service vulnerabilities?
# Do you believe that not handling rejections may cause memory leaks or denial of service vulnerabilities?

this is a very leading question; if the third option is actually an answer, then it is not a given that this is the case, which means "are you aware that" is inappropriate, since it implies that this is objectively the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's wait for @mcollina reply above. We can remove this question if it's considered too leading/biased/argumentative.

Copy link
Member

Choose a reason for hiding this comment

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

I find @ljharb 's suggestions equally leading by the way :]

I recommend we ask something like:

Suggested change
# Are you aware that not handling rejections may cause memory leaks or denial of service vulnerabilities?
# How do you deal with managing resources wrapped by promises when an unhandled rejection occurs?

Or something similar that indicates that the issue is:

  • I have a handle - for example a database connection.
  • the method containing that handle throws (for example releasing the connection).
  • Since it's an async method (if it was sync the server would crash) - I get an unhandled rejection.
  • If I don't quit or explicitly deal with it, it's super easy to create cascading failures and cause denial of service vulnerabilities.

I think it's important that this is not all unhandled rejections but a particular sort, but on the other hand that this is a very real issue.

Copy link
Member

Choose a reason for hiding this comment

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

I agree - since it's likely a small number of unhandled rejection cases, that if they occur, can cause very serious problems, it's important to avoid leading questions in any direction.

@mmarchini
Copy link
Contributor Author

Sorry y'all for the delay updating the survey. Thank you @benjamingr @devsnek and @ljharb for this round of feedback! I think I incorporated everything in this last commit:

  1. Added a preface explaining what rejections and unhandled rejections are (with examples)
  2. Expanded some questions to cover more cases
  3. Removed the last question as I couldn't think of a way to phrase the question + answers without somehow leading the participant. If anyone has suggestions on how to rephrase it, we can re-add it. I think the title suggested by @benjamingr here would be good, but I'm having trouble phrasing answers for it, maybe this should be an text field question where users can describe what they do?

I think we should start to wrap this up so we can send it out and gather results.

mmarchini and others added 9 commits May 13, 2020 17:58
Signed-off-by: Matheus Marchini <mmarchini@netflix.com>
Co-Authored-By: Rich Trott <rtrott@gmail.com>
Co-Authored-By: Andrey Pechkurov <37772591+puzpuzpuz@users.noreply.github.com>
Co-Authored-By: James M Snell <jasnell@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Overall, this looks great, and seems to avoid the bias I inferred from the initial drafts. Nice work!

surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
surveys/promise-rejections/survey.md Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Removed the last question as I couldn't think of a way to phrase the question + answers without somehow leading the participant. If anyone has suggestions on how to rephrase it, we can re-add it. I think the title suggested by @benjamingr here would be good, but I'm having trouble phrasing answers for it, maybe this should be an text field question where users can describe what they do?

I would like the last question to be added again, and I was surprised it was removed without cc me (I requested it to be added).. I think it’s a critical question for the validity of this survey as the vast majority of developers do not known that they can be the cause of leaks.

(last one I was talking to had 30 unhandled rejections per minute and they have massive leak issues).

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

mmarchini added a commit that referenced this pull request May 25, 2020
Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: #857
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mmarchini
Copy link
Contributor Author

Landed in 83dc496

@nylen
Copy link

nylen commented May 28, 2020

Who does this survey get sent out to? I'd like to participate.

@nylen
Copy link

nylen commented May 28, 2020

Also, the fact that a survey is being done at all seems to contradict what the Node.js command-line output says:

(node:12314) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code

Sounds like the decision was already made some time ago?

@dfabulich
Copy link
Contributor

Sounds like the decision was already made some time ago?

As of today, TSC members are in disagreement about it, regardless of what the deprecation message said when it was written. We're hoping that this survey will help TSC to reach consensus.

@mmarchini
Copy link
Contributor Author

@nylen this is a very long discussion which spans across multiple issues and pull requests on several repositories in the Node.js org. Decisions can be revisited, and we have in the past reverted deprecations instead of moving forward with them. There's no consensus on what the default should be, and not everyone agrees that an unhandled rejection should crash the process. The TSC will need to vote on this issue, but we want to better understand how users are using promises and what the expected behavior users expect Node.js to adopt, before going to a vote.

I'll share the link to the survey here once it' ready. The OpenJS Foundation will be responsible for widespread distribution of the survey.

@mmarchini
Copy link
Contributor Author

@dfabulich TSC members being in disagreement is not a problem, since conflict is resolved via vote. But we want to hear the community since this is a sensitive topic.

@dfabulich
Copy link
Contributor

It's may be too late, but is there any chance we could call it warn-with-error-code? This name emerged while reviewing nodejs/node#33475 and I think it's a little clearer.

(It's OK if the survey goes out as-is, but it would definitely be nicer to tweak this one last thing.)

@mmarchini
Copy link
Contributor Author

For the purpose of the survey I don't think the name matters that much, since we explain what each of the modes are (and we listed modes which are not part of Node.js yet). But there's still time to change it, yes.

@nylen
Copy link

nylen commented Jul 8, 2020

@mmarchini has this survey started yet?

@mmarchini
Copy link
Contributor Author

No. I forwarded the request to the Foundation, but they don't have an ETA yet. Either way, this is a breaking change, therefore even if it lands on master soon it won't be included in any release before we release v15.0.0 in October, so we have time to run the survey and reach a decision.

@dfabulich
Copy link
Contributor

When the survey goes out, should I expect to see a comment here? I just want to make sure I don't miss the announcement.

@mmarchini
Copy link
Contributor Author

Yes, I'll share a comment here and on the pull requests and issues.

@dfabulich
Copy link
Contributor

It's been another whole month. I think it might be time to pester someone again?

@mmarchini
Copy link
Contributor Author

I'm in contact with them on the OpenJS foundation slack channel. Let me ping them again to see how the progress is going,

@mmarchini
Copy link
Contributor Author

The survey is available at: https://www.surveymonkey.com/r/FTJM7YD

We also wrote an accompanying blog post for extra context: https://medium.com/@nodejs/node-js-promise-reject-use-case-survey-98e3328340c9

The survey will run for at least two weeks, at which point we'll evaluate if the number of replies is enough for us to move forward, otherwise we might extend it for a week or two. Please fill out the survey as it will help us decide the future of unhandled promise rejections on Node.js!

@nylen
Copy link

nylen commented Aug 18, 2020

Thanks for linking to the survey. There is no place for additional comments apart from the "Other" field so I will leave them here.

Please take the following into account regardless of the decision ultimately (re)made by the TSC:

@benjamingr
Copy link
Member

@nylen note: I think that message comes off as needlessly aggressive. By making the word "must" bold you are not doing yourself a service.

Instead:

I am thankful for your involvement and engagement in this space (we need more people who understand this problem domain involved). Please if at all possible keep discussions friendly (honestly it's very silly to not have friendly discussions and getting along given how few people are involved and how historically most of the problems were solved by discussions). We did have quite a few people walk away from the problem space in the past because of the way conversations were held.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.