FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: check HasCaught() in JSStream calls by addaleax · Pull Request #26124 · nodejs/node · GitHub

/ node Public

src: check HasCaught() in JSStream calls - #26124

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:worker-js-streams-terminate
Closed

src: check HasCaught() in JSStream calls#26124
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:worker-js-streams-terminate

Conversation

addaleax commented Feb 15, 2019
edited
Loading

Copy link
Copy Markdown
Member

MakeCallback can return an empty MaybeLocal<> even if no
exception has been generated, in particular, if
we were already terminating the current thread before the TryCatch
scope started, which meant it would not have an exception to report.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Copy link
Copy Markdown
Collaborator

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 15, 2019
addaleax added the worker Issues and PRs related to Worker support. label Feb 15, 2019
`MakeCallback` can return an empty `MaybeLocal<>` even if no
exception has been generated, in particular, if
we were already terminating the current thread *before* the `TryCatch`
scope started, which meant it would not have an exception to report.
addaleax force-pushed the worker-js-streams-terminate branch from bf29050 to 0822101 Compare February 15, 2019 11:15
addaleax changed the title src: use CatchMode::kFatal in JSStream calls src: check HasCaught() in JSStream calls Feb 15, 2019

Copy link
Copy Markdown
Member Author

jasnell commented Feb 15, 2019

Copy link
Copy Markdown
Member

Good...um... catch :-)

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 15, 2019

Copy link
Copy Markdown
Member Author

Copy link
Copy Markdown
Member Author

Landed in 586318a

addaleax closed this Feb 17, 2019
addaleax deleted the worker-js-streams-terminate branch February 17, 2019 17:08
addaleax added a commit that referenced this pull request Feb 17, 2019
`MakeCallback` can return an empty `MaybeLocal<>` even if no
exception has been generated, in particular, if
we were already terminating the current thread *before* the `TryCatch`
scope started, which meant it would not have an exception to report.

PR-URL: #26124
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Feb 17, 2019
`MakeCallback` can return an empty `MaybeLocal<>` even if no
exception has been generated, in particular, if
we were already terminating the current thread *before* the `TryCatch`
scope started, which meant it would not have an exception to report.

PR-URL: #26124
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Feb 26, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
`MakeCallback` can return an empty `MaybeLocal<>` even if no
exception has been generated, in particular, if
we were already terminating the current thread *before* the `TryCatch`
scope started, which meant it would not have an exception to report.

PR-URL: #26124
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL