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.

Sign up for GitHub

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

Jump to bottom

process: Cast promise rejection reason to string #11640

Closed

apexskier wants to merge 2 commits into nodejs:master from apexskier:promise-symbol-rejection

Contributor

apexskier commented Mar 1, 2017 •

edited

Loading

The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
~~[ ] documentation is changed or added~~
commit message follows commit guidelines

Affected core subsystem(s)

process

cjihrig previously requested changes

View reviewed changes

Contributor

cjihrig left a comment

I don't think we need a message test for this. A "normal" test in test/parallel/ that uses common.expectWarning() should do.

mscdex added the process label

addaleax added the promises label

Contributor

Fishrock123 commented Mar 2, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6664/

Member

jasnell commented Mar 2, 2017

Definitely agree that all this needs is a simple common.expectsWarning() check.

Contributor Author

apexskier commented Mar 2, 2017

From the source and my tests, it looks like common.expectWarning() will only work if a single warning is generated in the test.

A test like the following generates two warnings with different names, so I can't ensure the warning matches. I could

'use strict';
// ensure this doesn't crash
Promise.reject(Symbol());

(node:95244) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Symbol()
(node:95244) [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.

I've got a change that allows expectWarning to take a map of names to message(s) or the original parameter types.

Contributor

cjihrig commented Mar 13, 2017

Can you split the changes to test/common.js into a separate commit.

apexskier added 2 commits

March 13, 2017 09:09


          test: support multiple warnings in common.checkWarning

89e59e0

This allows the common.checkWarning() test method to accept a map of
warning names to description(s), to allow testing code that generates
multiple types of warnings.


          process: Cast promise rejection reason to string

840021c

The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637

Fishrock123 self-assigned this

refack force-pushed the master branch from 16073c0 to fbe946b Compare

April 14, 2017 04:11

addaleax approved these changes

View reviewed changes

Member

addaleax left a comment

CI: https://ci.nodejs.org/job/node-test-pull-request/7748/

addaleax dismissed cjihrig’s stale review

April 29, 2017 21:22

request to not have a message test has been addressed

Member

addaleax commented Apr 29, 2017

Landed in 765be6c...a3132b0, thanks for the PR!

addaleax closed this

addaleax pushed a commit that referenced this pull request


          test: support multiple warnings in checkWarning

74f61e8

This allows the common.checkWarning() test method to accept a map of
warning names to description(s), to allow testing code that generates
multiple types of warnings.

PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

addaleax pushed a commit that referenced this pull request


          process: cast promise rejection reason to string

a3132b0

The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

evanlucas mentioned this pull request

v7.10.0 proposal #12775

Merged

evanlucas pushed a commit that referenced this pull request


          test: support multiple warnings in checkWarning

59efb0b

This allows the common.checkWarning() test method to accept a map of
warning names to description(s), to allow testing code that generates
multiple types of warnings.

PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

evanlucas pushed a commit that referenced this pull request


          process: cast promise rejection reason to string

The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

bakkot commented May 2, 2017

Symbol is only one example among many of something which throws when trying to call .toString. If I read this correctly, this PR doesn't fix the core issue, and will presumably still crash on e.g.

Promise.reject({toString(){throw 0;}})

bakkot mentioned this pull request

Regarding "Don't break user intuition" tc39/proposal-bigint#30

Closed

evanlucas pushed a commit that referenced this pull request


          test: support multiple warnings in checkWarning

2331c9b

This allows the common.checkWarning() test method to accept a map of
warning names to description(s), to allow testing code that generates
multiple types of warnings.

PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

evanlucas pushed a commit that referenced this pull request


          process: cast promise rejection reason to string

c4062c1

The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

evanlucas added a commit that referenced this pull request


          2017-05-02, Version 7.10.0 (Current)

e0dd758

Notable changes:

* **crypto**:
  - add randomFill and randomFillSync (Evan Lucas)
    #10209
* **meta**: Added new collaborators
  - add lucamaraschi to collaborators (Luca Maraschi)
    #12538
  - add DavidCai1993 to collaborators (David Cai)
    #12435
  - add jkrems to collaborators (Jan Krems)
    #12427
  - add AnnaMag to collaborators (AnnaMag)
    #12414
* **process**:
  - fix crash when Promise rejection is a Symbol (Cameron Little)
    #11640
* **url**:
  - make WHATWG URL more spec compliant (Timothy Gu)
    #12507
* **v8**:
  - fix stack overflow in recursive method (Ben Noordhuis)
    #12460
  - fix build errors with g++ 7 (Ben Noordhuis)
    #12392

PR-URL: #12775

evanlucas pushed a commit that referenced this pull request


          test: support multiple warnings in checkWarning

bace499

This allows the common.checkWarning() test method to accept a map of
warning names to description(s), to allow testing code that generates
multiple types of warnings.

PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

evanlucas pushed a commit that referenced this pull request


          process: cast promise rejection reason to string

a46e63b

The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

evanlucas added a commit that referenced this pull request


          2017-05-02, Version 7.10.0 (Current)

84ed5f8

Notable changes:

* **crypto**:
  - add randomFill and randomFillSync (Evan Lucas)
    #10209
* **meta**: Added new collaborators
  - add lucamaraschi to collaborators (Luca Maraschi)
    #12538
  - add DavidCai1993 to collaborators (David Cai)
    #12435
  - add jkrems to collaborators (Jan Krems)
    #12427
  - add AnnaMag to collaborators (AnnaMag)
    #12414
* **process**:
  - fix crash when Promise rejection is a Symbol (Cameron Little)
    #11640
* **url**:
  - make WHATWG URL more spec compliant (Timothy Gu)
    #12507
* **v8**:
  - fix stack overflow in recursive method (Ben Noordhuis)
    #12460
  - fix build errors with g++ 7 (Ben Noordhuis)
    #12392

PR-URL: #12775

evanlucas pushed a commit that referenced this pull request


          test: support multiple warnings in checkWarning

d78adcc

This allows the common.checkWarning() test method to accept a map of
warning names to description(s), to allow testing code that generates
multiple types of warnings.

PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

evanlucas pushed a commit that referenced this pull request


          process: cast promise rejection reason to string

cd54208

The unhandled promise rejection warning uses a template literal and
prints the reason a promise was rejected. If rejecting with a symbol,
the symbol failed to convert to a string and the process crashed. Now,
symbols are casted to strings and the process does not crash.

Fixes: #11637
PR-URL: #11640
Reviewed-By: Anna Henningsen <[email protected]>

evanlucas added a commit that referenced this pull request


          2017-05-02, Version 7.10.0 (Current)

b5e70ae

Notable changes:

* **crypto**:
  - add randomFill and randomFillSync (Evan Lucas)
    #10209
* **meta**: Added new collaborators
  - add lucamaraschi to collaborators (Luca Maraschi)
    #12538
  - add DavidCai1993 to collaborators (David Cai)
    #12435
  - add jkrems to collaborators (Jan Krems)
    #12427
  - add AnnaMag to collaborators (AnnaMag)
    #12414
* **process**:
  - fix crash when Promise rejection is a Symbol (Cameron Little)
    #11640
* **url**:
  - make WHATWG URL more spec compliant (Timothy Gu)
    #12507
* **v8**:
  - fix stack overflow in recursive method (Ben Noordhuis)
    #12460
  - fix build errors with g++ 7 (Ben Noordhuis)
    #12392

PR-URL: #12775

evanlucas added a commit that referenced this pull request


          2017-05-02, Version 7.10.0 (Current)

871fb5a

Notable changes:

* **crypto**:
  - add randomFill and randomFillSync (Evan Lucas)
    #10209
* **meta**: Added new collaborators
  - add lucamaraschi to collaborators (Luca Maraschi)
    #12538
  - add DavidCai1993 to collaborators (David Cai)
    #12435
  - add jkrems to collaborators (Jan Krems)
    #12427
  - add AnnaMag to collaborators (AnnaMag)
    #12414
* **process**:
  - fix crash when Promise rejection is a Symbol (Cameron Little)
    #11640
* **url**:
  - make WHATWG URL more spec compliant (Timothy Gu)
    #12507
* **v8**:
  - fix stack overflow in recursive method (Ben Noordhuis)
    #12460
  - fix build errors with g++ 7 (Ben Noordhuis)
    #12392

PR-URL: #12775

imyller added a commit to imyller/meta-nodejs that referenced this pull request


          2017-05-02, Version 7.10.0 (Current)

eec531e

    Notable changes:

    * **crypto**:
      - add randomFill and randomFillSync (Evan Lucas)
        nodejs/node#10209
    * **meta**: Added new collaborators
      - add lucamaraschi to collaborators (Luca Maraschi)
        nodejs/node#12538
      - add DavidCai1993 to collaborators (David Cai)
        nodejs/node#12435
      - add jkrems to collaborators (Jan Krems)
        nodejs/node#12427
      - add AnnaMag to collaborators (AnnaMag)
        nodejs/node#12414
    * **process**:
      - fix crash when Promise rejection is a Symbol (Cameron Little)
        nodejs/node#11640
    * **url**:
      - make WHATWG URL more spec compliant (Timothy Gu)
        nodejs/node#12507
    * **v8**:
      - fix stack overflow in recursive method (Ben Noordhuis)
        nodejs/node#12460
      - fix build errors with g++ 7 (Ben Noordhuis)
        nodejs/node#12392

    PR-URL: nodejs/node#12775

Signed-off-by: Ilkka Myller <[email protected]>

anchnk pushed a commit to anchnk/node that referenced this pull request


          2017-05-02, Version 7.10.0 (Current)

575955f

Notable changes:

* **crypto**:
  - add randomFill and randomFillSync (Evan Lucas)
    nodejs#10209
* **meta**: Added new collaborators
  - add lucamaraschi to collaborators (Luca Maraschi)
    nodejs#12538
  - add DavidCai1993 to collaborators (David Cai)
    nodejs#12435
  - add jkrems to collaborators (Jan Krems)
    nodejs#12427
  - add AnnaMag to collaborators (AnnaMag)
    nodejs#12414
* **process**:
  - fix crash when Promise rejection is a Symbol (Cameron Little)
    nodejs#11640
* **url**:
  - make WHATWG URL more spec compliant (Timothy Gu)
    nodejs#12507
* **v8**:
  - fix stack overflow in recursive method (Ben Noordhuis)
    nodejs#12460
  - fix build errors with g++ 7 (Ben Noordhuis)
    nodejs#12392

PR-URL: nodejs#12775

Contributor

sam-github commented May 15, 2017

Should this land on 6.x?

It sounds like it fixes a crash, so should be, but it also sounds like a change in error messages.

gibfahn mentioned this pull request

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

Member

gibfahn commented Jun 18, 2017

Should this land on 6.x?

ping

Member

addaleax commented Jun 18, 2017

Should this land on 6.x?

@gibfahn Yes, it should. :)

bakkot mentioned this pull request

Unhandled promise rejection with throwing toString crashes node #13771

Closed

bakkot commented Jun 18, 2017

people backporting this: see my above comment, which I just opened a new issue for at #13771. The fix in this PR doesn't actually handle the full extent of the problem.

Member

gibfahn commented Jun 18, 2017

The fix in this PR doesn't actually handle the full extent of the problem.

Does it make things worse? As long as it doesn't regess things (and hopefully is an improvement) then it should be fine to backport.

bakkot commented Jun 18, 2017

It's definitely an improvement, just not a complete solution. And I'd expect a complete solution to subsume this one.

gibfahn added the backport-requested-v6.x label

Member

gibfahn commented Jun 18, 2017

It's definitely an improvement, just not a complete solution. And I'd expect a complete solution to subsume this one.

Sure, but any "complete solution" will be a commit on top of this one, so if we don't backport this first we'll get merge conflicts when we try to cherry-pick the complete solution.

Member

gibfahn commented Jun 18, 2017

Is anyone willing to backport this to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

TimothyGu mentioned this pull request

promises: more robust stringification #13784

Closed

3 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

process promises