Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix JS Stream Socket finishShutdown crash#49400

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

pimterry
Copy link
Member

@pimterrypimterry commentedAug 29, 2023•
edited
Loading

Thisfixes#48519
EDIT: andfixes#46094

This is independent of my other relatedfix#49327 but in most relevant scenarios you'll want both together. Each of these PRs has a test that catastrophically fails right now, but works correctly with the corresponding change included, but you'll likely want both since in production I find that either fix by itself ends up resulting in the other error crashing my application instead a little later on (i.e. these two separate issues are currently racing to crash my code).

There's a detailed breakdown of the crash fixed in this PRhere. To summarize:

  • A JS Stream Socket (JSS) is created, wrapping an HTTP/2 stream (anything might work) and then used by a TLS stream (i.e. TLS runs on top of the JSS) withallowHalfOpen = false set (this is common for TLS, AFAICT).
  • The underlying H2 stream closes
  • This fires a 'close' event, which results in the JSS callingdoClose, and also ends the TLS stream (which in turn closes the writable side, due toallowHalfOpen = false) which callsdoShutdown on the JSS. Effectively both sides try to shut down the JSS at the same time.
  • doClose runs first, callsthis.stream.destroy() which setsthis._handle to null, and schedules asetImmediate to cancel any pending writes or shutdowns.
  • doShutdown then runs, picks upthis._handle (now null), and then schedules afinishShutdown that will callthis._handle.finishShutdown (=uncatchable NPE)
  • Before9ccf8b2 (released in Node 18+) thedoClose callback ran first, clearedkCurrentShutdownRequest, and so the null pointer was never actually read, but effectively just by luck.
  • After that change, thefinishShutdown fromdoShutdown runs first, and sodoes use its null pointer every time, crashing everything.

Rather than just reverting tosetImmediate or usingprocess.nextTick indoClose too, I've fixed this properly: now, if there is any race withdoClose, the other methods recognize it, store thekCurrent{Shutdown,Write}Request param to allowdoClose to do its clean cancellation a moment later, and then they avoid reading or using the nullthis._handle value entirely.

That above example explains thedoShutdown race. A similar race applies todoWrite/finishWrite too, which I can reliably reproduce in my application after fixing just the shutdown case, although I don't have a reliable test to repro this (#46094,#35695,#27258 andmicrosoft/vscode#188676 all appear to be examples of the same issue). With the equivalent fix included fordoWrite here too, I can no longer reproduce that error either.

With the fixes for both methods here plus#49327, I can proxy significant chunks of real HTTP/2 browser traffic through my app, including all the weird edge cases and errors of the web for at least an hour. Previously this crashed comfortably within a minute on Node 18+.

This also adds asserts to both affected methods, to more explicitly catch any other cases where this could happen (in both cases, if those asserts fail, it's very likely that there'll be an NPE next stick in the corresponding finishX methods).

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-botnodejs-github-bot added the needs-ciPRs that need a full CI run. labelAug 29, 2023
@pimterrypimterryforce-pushed thefix-js-stream-socket-finishShutdown-crash branch from8851f62 to3131da6CompareAugust 29, 2023 10:06
A JS stream socket wraps a stream, exposing it as a socket for somethingon top which needs a socket specifically (e.g. an HTTP server).If the internal stream is closed in the same tick as the layer on topattempts to close this stream, the race between doShutdown and doCloseresults in an uncatchable exception. A similar race can happen withdoClose and doWrite.It seems legitimate these can happen in parallel, so this resolves thatby explicitly detecting and handling that situation: if a close is inprogress, both doShutdown & doWrite allow doClose to runfinishShutdown/Write for them, cancelling the operation, without tryingto use this._handle (which will be null) in the meantime.
@pimterrypimterryforce-pushed thefix-js-stream-socket-finishShutdown-crash branch from3131da6 tofb9611bCompareAugust 29, 2023 10:18
@lpincalpinca added the request-ciAdd this label to start a Jenkins CI on a PR. labelAug 29, 2023
@github-actionsgithub-actionsbot removed the request-ciAdd this label to start a Jenkins CI on a PR. labelAug 29, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@pimterry
Copy link
MemberAuthor

There's two CI failures here:

  • node-test-commit-arm:sequential.test-single-executable-application-snapshot-and-code-cache
    That seems likely unrelated, and there's at leastone other random failure of that test recently.
  • win2012r2-vs2019-COMPILED_BY-vs2019-x86:InspectorSocketTest.HostIPv6NonRoutable
    As a networking test, that plausiblycould be related, but it did also fail around the same time on another unrelated commit (here) so maybe not? I'm not sure how to reproduce that locally unfortunately. Would be good if somebody could rerun that, and I'll try to investigate it further if it's still failing.

@nodejs-github-bot
Copy link
Collaborator

@pimterry
Copy link
MemberAuthor

Ok, that's somewhat clearer:

  • The Windows tests now all pass happily, so that failure doesn't seem related
  • AnOSX test now fails for very unrelated reasons ("not enough disk space for writing...")
  • The ARM test isstill failing but now with a different test failing. Since that test passed last time, and doesn't seem to use networking at all, I think we can confidently say that's not related to this either.

Given that, I think that means this is good to go, and CI just needs nudging until it passes. I'll leave it here, but let me know if there's anything else I can do to help get this merged.

@nodejs-github-bot
Copy link
Collaborator

@mcollinamcollina added author readyPRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ciPRs that need a full CI run. labelsAug 30, 2023
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollinamcollina added the commit-queueAdd this label to land a pull request using GitHub Actions. labelAug 30, 2023
@nodejs-github-botnodejs-github-bot added commit-queue-failedAn error occurred while landing this pull request using GitHub Actions. and removed commit-queueAdd this label to land a pull request using GitHub Actions. labelsAug 31, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/49400✔  Done loading data for nodejs/node/pull/49400----------------------------------- PR info ------------------------------------Title      Fix JS Stream Socket finishShutdown crash (#49400)   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!Branch     pimterry:fix-js-stream-socket-finishShutdown-crash -> nodejs:mainLabels     author readyCommits    2 - net: fix crash due to simultaneous close/shutdown on JS Stream Sockets - net: use asserts in JS Socket Stream to catch races in futureCommitters 1 - Tim Perry PR-URL: https://github.com/nodejs/node/pull/49400Reviewed-By: Matteo Collina Reviewed-By: Luigi Pinca ------------------------------ Generated metadata ------------------------------PR-URL: https://github.com/nodejs/node/pull/49400Reviewed-By: Matteo Collina Reviewed-By: Luigi Pinca --------------------------------------------------------------------------------   ℹ  This PR was created on Tue, 29 Aug 2023 10:04:35 GMT   ✔  Approvals: 2   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49400#pullrequestreview-1603592553   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/49400#pullrequestreview-1600355896   ✔  Last GitHub CI successful   ℹ  Last Full PR CI on 2023-08-30T16:27:57Z: https://ci.nodejs.org/job/node-test-pull-request/53647/- Querying data for job/node-test-pull-request/53647/   ✔  Last Jenkins CI successful--------------------------------------------------------------------------------   ✔  No git cherry-pick in progress   ✔  No git am in progress   ✔  No git rebase in progress--------------------------------------------------------------------------------- Bringing origin/main up to date...From https://github.com/nodejs/node * branch                  main       -> FETCH_HEAD✔  origin/main is now up-to-date- Downloading patch for 49400From https://github.com/nodejs/node * branch                  refs/pull/49400/merge -> FETCH_HEAD✔  Fetched commits as b781eaf4309a..fb9611bd5d29--------------------------------------------------------------------------------[main 775d624f19] net: fix crash due to simultaneous close/shutdown on JS Stream Sockets Author: Tim Perry  Date: Thu Aug 24 16:05:02 2023 +0100 2 files changed, 91 insertions(+) create mode 100644 test/parallel/test-http2-client-connection-tunnelling.js[main 316fc7cb68] net: use asserts in JS Socket Stream to catch races in future Author: Tim Perry  Date: Fri Aug 25 14:16:35 2023 +0100 1 file changed, 3 insertions(+)   ✔  Patches appliedThere are 2 commits in the PR. Attempting autorebase.Rebasing (2/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
net: fix crash due to simultaneous close/shutdown on JS Stream Sockets
A JS stream socket wraps a stream, exposing it as a socket for something
on top which needs a socket specifically (e.g. an HTTP server).
If the internal stream is closed in the same tick as the layer on top
attempts to close this stream, the race between doShutdown and doClose
results in an uncatchable exception. A similar race can happen with
doClose and doWrite.
It seems legitimate these can happen in parallel, so this resolves that
by explicitly detecting and handling that situation: if a close is in
progress, both doShutdown & doWrite allow doClose to run
finishShutdown/Write for them, cancelling the operation, without trying
to use this._handle (which will be null) in the meantime.
PR-URL:#49400
Reviewed-By: Matteo Collinamatteo.collina@gmail.com
Reviewed-By: Luigi Pincaluigipinca@gmail.com
[detached HEAD 189fd82de7] net: fix crash due to simultaneous close/shutdown on JS Stream Sockets
Author: Tim Perrypimterry@gmail.com
Date: Thu Aug 24 16:05:02 2023 +0100
2 files changed, 91 insertions(+)
create mode 100644 test/parallel/test-http2-client-connection-tunnelling.js
Rebasing (3/4)
Rebasing (4/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
net: use asserts in JS Socket Stream to catch races in future
PR-URL:#49400
Reviewed-By: Matteo Collinamatteo.collina@gmail.com
Reviewed-By: Luigi Pincaluigipinca@gmail.com
[detached HEAD a803862f7d] net: use asserts in JS Socket Stream to catch races in future
Author: Tim Perrypimterry@gmail.com
Date: Fri Aug 25 14:16:35 2023 +0100
1 file changed, 3 insertions(+)
Successfully rebased and updated refs/heads/main.
ℹ  Addcommit-queue-squash label to land the PR as one commit, orcommit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/6035974708

@mcollinamcollina added commit-queueAdd this label to land a pull request using GitHub Actions. commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue-failedAn error occurred while landing this pull request using GitHub Actions. labelsAug 31, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queueAdd this label to land a pull request using GitHub Actions. labelAug 31, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed inf863117...47add7e

pimterry reacted with hooray emoji

UlisesGascon pushed a commit that referenced this pull requestSep 10, 2023
PR-URL:#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@UlisesGasconUlisesGascon mentioned this pull requestSep 10, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull requestNov 1, 2023
A JS stream socket wraps a stream, exposing it as a socket for somethingon top which needs a socket specifically (e.g. an HTTP server).If the internal stream is closed in the same tick as the layer on topattempts to close this stream, the race between doShutdown and doCloseresults in an uncatchable exception. A similar race can happen withdoClose and doWrite.It seems legitimate these can happen in parallel, so this resolves thatby explicitly detecting and handling that situation: if a close is inprogress, both doShutdown & doWrite allow doClose to runfinishShutdown/Write for them, cancelling the operation, without tryingto use this._handle (which will be null) in the meantime.PR-URL:nodejs#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull requestNov 1, 2023
PR-URL:nodejs#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
codebytere added a commit to electron/electron that referenced this pull requestNov 6, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 14, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 15, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 16, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 21, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 22, 2023
targos pushed a commit that referenced this pull requestNov 27, 2023
A JS stream socket wraps a stream, exposing it as a socket for somethingon top which needs a socket specifically (e.g. an HTTP server).If the internal stream is closed in the same tick as the layer on topattempts to close this stream, the race between doShutdown and doCloseresults in an uncatchable exception. A similar race can happen withdoClose and doWrite.It seems legitimate these can happen in parallel, so this resolves thatby explicitly detecting and handling that situation: if a close is inprogress, both doShutdown & doWrite allow doClose to runfinishShutdown/Write for them, cancelling the operation, without tryingto use this._handle (which will be null) in the meantime.PR-URL:#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull requestNov 27, 2023
PR-URL:#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
codebytere added a commit to electron/electron that referenced this pull requestNov 28, 2023
@targostargos mentioned this pull requestNov 28, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 29, 2023
jkleinsc pushed a commit to electron/electron that referenced this pull requestNov 30, 2023
* chore: upgrade to Node.js v20* src: allow embedders to override NODE_MODULE_VERSIONnodejs/node#49279* src: fix missing trailing ,nodejs/node#46909* src,tools: initialize cppgcnodejs/node#45704* tools: allow passing absolute path of config.gypi in js2cnodejs/node#49162* tools: port js2c.py to C++nodejs/node#46997* doc,lib: disambiguate the old term, NativeModulenodejs/node#45673* chore: fixup Node.js BSSL tests*nodejs/node#49492*nodejs/node#44498* deps: upgrade to libuv 1.45.0nodejs/node#48078* deps: update V8 to 10.7nodejs/node#44741* test: use gcUntil() in test-v8-serialize-leaknodejs/node#49168* module: make CJS load from ESM loadernodejs/node#47999* src: make BuiltinLoader threadsafe and non-globalnodejs/node#45942* chore: address changes to CJS/ESM loading* module: make CJS load from ESM loader (nodejs/node#47999)* lib: improve esm resolve performance (nodejs/node#46652)* bootstrap: optimize modules loaded in the built-in snapshotnodejs/node#45849* test: mark test-runner-output as flakynodejs/node#49854* lib: lazy-load deps in modules/run_main.jsnodejs/node#45849* url: use private properties for brand checknodejs/node#46904* test: refactor `test-node-output-errors`nodejs/node#48992* assert: deprecate callTrackernodejs/node#47740* src: cast v8::Object::GetInternalField() return value to v8::Valuenodejs/node#48943* test: adapt test-v8-stats for V8 updatenodejs/node#45230* tls: ensure TLS Sockets are closed if the underlying wrapclosesnodejs/node#49327* test: deflake test-tls-socket-closenodejs/node#49575* net: fix crash due to simultaneous close/shutdown on JS Stream Socketsnodejs/node#49400* net: use asserts in JS Socket Stream to catch races in futurenodejs/node#49400* lib: fix BroadcastChannel initialization locationnodejs/node#46864* src: create BaseObject with node::Realmnodejs/node#44348* src: implement DataQueue and non-memory resident Blobnodejs/node#45258* sea: add support for V8 bytecode-only cachingnodejs/node#48191* chore: fixup patch indices* gyp: put filenames in variablesnodejs/node#46965* build: modify js2c.py into GN executable* fix: (WIP) handle string replacement of fs -> original-fs* [v20.x] backport vm-related memoryfixesnodejs/node#49874* src: make BuiltinLoader threadsafe and non-globalnodejs/node#45942* src: avoid copying string in fs_permissionnodejs/node#47746* look upon my works ye mightyand dispair* chore: patch cleanup* [api] Remove AllCan Read/Writehttps://chromium-review.googlesource.com/c/v8/v8/+/5006387* fix: missing include for NODE_EXTERN* chore: fixup patch indices* fix: fail properly when js2c fails in Node.js* build: fix js2c root_gen_dir* fix: lib/fs.js -> lib/original-fs.js* build: fix original-fs file xforms* fixup! module: make CJS load from ESM loader* build: get rid of CppHeap for now* build: add patch to prevent extra fs lookup on esm load* build: greatly simplify js2c modificationsMoves our original-fs modifications back into a super simple python script action, wires up the output of that action into our call to js2c* chore: update to handle moved internal/modules/helpers file* test: update @types/node test* feat: enable preventing cppgc heap creation* feat: optionally prevent calling V8::EnableWebAssemblyTrapHandler* fix: no cppgc initialization in the renderer* gyp: put filenames in variablesnodejs/node#46965* test: disable single executable tests* fix: nan tests failing on node headers missing file* tls,http2: send fatal alert on ALPN mismatchnodejs/node#44031* test: disable snapshot tests*nodejs/node#47887*nodejs/node#49684*nodejs/node#44193* build: use deps/v8 for v8/toolsNode.js hard depends on these in their builtins* test: fix edge snapshot stack tracesnodejs/node#49659* build: remove js2c //base dep* build: use electron_js2c_toolchain to build node_js2c* fix: don't create SafeSet outside packageResolveFixes failure in parallel/test-require-delete-array-iterator:=== release test-require-delete-array-iterator ===Path: parallel/test-require-delete-array-iteratornode:internal/per_context/primordials:426    constructor(i) { super(i); } // eslint-disable-line no-useless-constructor                     ^TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator))    at new Set (<anonymous>)    at new SafeSet (node:internal/per_context/primordials:426:22)* fix: failing crashReporter tests on LinuxThese were failing because our change from node::InitializeNodeWithArgs tonode::InitializeOncePerProcess meant that we now inadvertently calledPlatformInit, which reset signal handling. This meant that our intentionalcrash function ElectronBindings::Crash no longer worked and the renderer processno longer crashed when process.crash() was called. We don't want to use Node.js'default signal handling in the renderer process, so we disable it by passingkNoDefaultSignalHandling to node::InitializeOncePerProcess.* build: only create cppgc heap on non-32 bit platforms* chore: clean up util:CompileAndCall* src: fix compatility with upcoming V8 12.1 APIsnodejs/node#50709* fix: use thread_local BuiltinLoader* chore: fixup v8 patch indices---------Co-authored-by: Keeley Hammond <vertedinde@electronjs.org>Co-authored-by: Samuel Attard <marshallofsound@electronjs.org>
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull requestDec 11, 2023
* chore: upgrade to Node.js v20* src: allow embedders to override NODE_MODULE_VERSIONnodejs/node#49279* src: fix missing trailing ,nodejs/node#46909* src,tools: initialize cppgcnodejs/node#45704* tools: allow passing absolute path of config.gypi in js2cnodejs/node#49162* tools: port js2c.py to C++nodejs/node#46997* doc,lib: disambiguate the old term, NativeModulenodejs/node#45673* chore: fixup Node.js BSSL tests*nodejs/node#49492*nodejs/node#44498* deps: upgrade to libuv 1.45.0nodejs/node#48078* deps: update V8 to 10.7nodejs/node#44741* test: use gcUntil() in test-v8-serialize-leaknodejs/node#49168* module: make CJS load from ESM loadernodejs/node#47999* src: make BuiltinLoader threadsafe and non-globalnodejs/node#45942* chore: address changes to CJS/ESM loading* module: make CJS load from ESM loader (nodejs/node#47999)* lib: improve esm resolve performance (nodejs/node#46652)* bootstrap: optimize modules loaded in the built-in snapshotnodejs/node#45849* test: mark test-runner-output as flakynodejs/node#49854* lib: lazy-load deps in modules/run_main.jsnodejs/node#45849* url: use private properties for brand checknodejs/node#46904* test: refactor `test-node-output-errors`nodejs/node#48992* assert: deprecate callTrackernodejs/node#47740* src: cast v8::Object::GetInternalField() return value to v8::Valuenodejs/node#48943* test: adapt test-v8-stats for V8 updatenodejs/node#45230* tls: ensure TLS Sockets are closed if the underlying wrapclosesnodejs/node#49327* test: deflake test-tls-socket-closenodejs/node#49575* net: fix crash due to simultaneous close/shutdown on JS Stream Socketsnodejs/node#49400* net: use asserts in JS Socket Stream to catch races in futurenodejs/node#49400* lib: fix BroadcastChannel initialization locationnodejs/node#46864* src: create BaseObject with node::Realmnodejs/node#44348* src: implement DataQueue and non-memory resident Blobnodejs/node#45258* sea: add support for V8 bytecode-only cachingnodejs/node#48191* chore: fixup patch indices* gyp: put filenames in variablesnodejs/node#46965* build: modify js2c.py into GN executable* fix: (WIP) handle string replacement of fs -> original-fs* [v20.x] backport vm-related memoryfixesnodejs/node#49874* src: make BuiltinLoader threadsafe and non-globalnodejs/node#45942* src: avoid copying string in fs_permissionnodejs/node#47746* look upon my works ye mightyand dispair* chore: patch cleanup* [api] Remove AllCan Read/Writehttps://chromium-review.googlesource.com/c/v8/v8/+/5006387* fix: missing include for NODE_EXTERN* chore: fixup patch indices* fix: fail properly when js2c fails in Node.js* build: fix js2c root_gen_dir* fix: lib/fs.js -> lib/original-fs.js* build: fix original-fs file xforms* fixup! module: make CJS load from ESM loader* build: get rid of CppHeap for now* build: add patch to prevent extra fs lookup on esm load* build: greatly simplify js2c modificationsMoves our original-fs modifications back into a super simple python script action, wires up the output of that action into our call to js2c* chore: update to handle moved internal/modules/helpers file* test: update @types/node test* feat: enable preventing cppgc heap creation* feat: optionally prevent calling V8::EnableWebAssemblyTrapHandler* fix: no cppgc initialization in the renderer* gyp: put filenames in variablesnodejs/node#46965* test: disable single executable tests* fix: nan tests failing on node headers missing file* tls,http2: send fatal alert on ALPN mismatchnodejs/node#44031* test: disable snapshot tests*nodejs/node#47887*nodejs/node#49684*nodejs/node#44193* build: use deps/v8 for v8/toolsNode.js hard depends on these in their builtins* test: fix edge snapshot stack tracesnodejs/node#49659* build: remove js2c //base dep* build: use electron_js2c_toolchain to build node_js2c* fix: don't create SafeSet outside packageResolveFixes failure in parallel/test-require-delete-array-iterator:=== release test-require-delete-array-iterator ===Path: parallel/test-require-delete-array-iteratornode:internal/per_context/primordials:426    constructor(i) { super(i); } // eslint-disable-line no-useless-constructor                     ^TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator))    at new Set (<anonymous>)    at new SafeSet (node:internal/per_context/primordials:426:22)* fix: failing crashReporter tests on LinuxThese were failing because our change from node::InitializeNodeWithArgs tonode::InitializeOncePerProcess meant that we now inadvertently calledPlatformInit, which reset signal handling. This meant that our intentionalcrash function ElectronBindings::Crash no longer worked and the renderer processno longer crashed when process.crash() was called. We don't want to use Node.js'default signal handling in the renderer process, so we disable it by passingkNoDefaultSignalHandling to node::InitializeOncePerProcess.* build: only create cppgc heap on non-32 bit platforms* chore: clean up util:CompileAndCall* src: fix compatility with upcoming V8 12.1 APIsnodejs/node#50709* fix: use thread_local BuiltinLoader* chore: fixup v8 patch indices---------Co-authored-by: Keeley Hammond <vertedinde@electronjs.org>Co-authored-by: Samuel Attard <marshallofsound@electronjs.org>
sercher added a commit to sercher/graaljs that referenced this pull requestApr 25, 2024
A JS stream socket wraps a stream, exposing it as a socket for somethingon top which needs a socket specifically (e.g. an HTTP server).If the internal stream is closed in the same tick as the layer on topattempts to close this stream, the race between doShutdown and doCloseresults in an uncatchable exception. A similar race can happen withdoClose and doWrite.It seems legitimate these can happen in parallel, so this resolves thatby explicitly detecting and handling that situation: if a close is inprogress, both doShutdown & doWrite allow doClose to runfinishShutdown/Write for them, cancelling the operation, without tryingto use this._handle (which will be null) in the meantime.PR-URL:nodejs/node#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull requestApr 25, 2024
PR-URL:nodejs/node#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull requestApr 25, 2024
A JS stream socket wraps a stream, exposing it as a socket for somethingon top which needs a socket specifically (e.g. an HTTP server).If the internal stream is closed in the same tick as the layer on topattempts to close this stream, the race between doShutdown and doCloseresults in an uncatchable exception. A similar race can happen withdoClose and doWrite.It seems legitimate these can happen in parallel, so this resolves thatby explicitly detecting and handling that situation: if a close is inprogress, both doShutdown & doWrite allow doClose to runfinishShutdown/Write for them, cancelling the operation, without tryingto use this._handle (which will be null) in the meantime.PR-URL:nodejs/node#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull requestApr 25, 2024
PR-URL:nodejs/node#49400Reviewed-By: Matteo Collina <matteo.collina@gmail.com>Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@mcollinamcollinamcollina approved these changes

@lpincalpincalpinca approved these changes

Assignees
No one assigned
Labels
author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Uncaught exception in socket code: "Cannot read properties of null (reading 'finishWrite')" Crash in JSStreamSocket.finishShutdown
4 participants
@pimterry@nodejs-github-bot@mcollina@lpinca
[8]ページ先頭
©2009-2025 Movatter.jp