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

[v12.x backport] deps: V8: cherry-pick eec10a2fd8fa and #34008 by Flarna · Pull Request #34776 · nodejs/node · GitHub

/ node Public

[v12.x backport] deps: V8: cherry-pick eec10a2fd8fa and #34008 - #34776

Closed
Flarna wants to merge 2 commits into
nodejs:v12.x-stagingfrom
dynatrace-oss-contrib:v12-thenables-v8-again
ConversationCommits (2)ChecksFiles changed
Closed

[v12.x backport] deps: V8: cherry-pick eec10a2fd8fa and #34008#34776
Flarna wants to merge 2 commits into
nodejs:v12.x-stagingfrom
dynatrace-oss-contrib:v12-thenables-v8-again

Conversation

Flarna commented Aug 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Backport #33778 and #34008 to v12.

There was already a backport PR (#34300) for #33778 but it was removed during creation of 12.18.3 as it looked like it breaks gulp tests on MacOs.

Later it turned out that gulp tests were broken already before therefore I see no reason anymore to not backport this to v12.

In this backport PR I added additionally #34008 which is just a new test to verify the backported fix.

see comments at #33778 (comment)

fyi @Qard, @MylesBorins

Refs: #33778
Refs: #34008
Refs: #34300
Refs: #34343

nodejs-github-bot added build Issues and PRs related to build files or the CI. v12.x v8 engine Issues and PRs related to the V8 dependency. labels Aug 14, 2020

MylesBorins commented Aug 14, 2020

Copy link
Copy Markdown
Contributor

@Flarna I think that we should do some tests before landing this and confirm where the breakages were coming from. While some of the tests were already broken #34008 ended up breaking more stuff unfortunately.

Qard approved these changes Aug 14, 2020
View reviewed changes

Qard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, assuming we can verify the source of the test issue, as Myles suggests.

Flarna commented Aug 14, 2020

Copy link
Copy Markdown
Member Author

started my first debug session on a Mac :o)
executed test of module glob-watcher. They fail consistent since 12.18.2 with an abort in uv_close. it's not always the same test so I assume it's some race.
With this PR included the probability seems to increase and it fails earlier.
The callstack looks equal with and without this PR:

* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff7fe982c2 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff7ff53bf1 libsystem_pthread.dylib`pthread_kill + 284
    frame #2: 0x00007fff7fe026a6 libsystem_c.dylib`abort + 127
    frame #3: 0x00007fff7fdcb20d libsystem_c.dylib`__assert_rtn + 324
    frame #4: 0x00000001010c7742 node`uv_close(handle=0x0000000105301988, close_cb=0x0000000000000000) at core.c:178:5
    frame #5: 0x0000000104fef195 fse.node`fse::FSEvents::Stop(Nan::FunctionCallbackInfo<v8::Value> const&) [inlined] fse::FSEvents::asyncStop(this=<unavailable>) at async.cc:42:3 [opt]
    frame #6: 0x0000000104fef17a fse.node`fse::FSEvents::Stop(info=0x00007ffeefbfe738) at methods.cc:33 [opt]
    frame #7: 0x0000000104ff059d fse.node`Nan::imp::FunctionCallbackWrapper(info=0x00007ffeefbfe7c0) at nan_callbacks_12_inl.h:176:3 [opt]
    frame #8: 0x00000001005b1ef8 node`v8::internal::FunctionCallbackArguments::Call(this=0x00007ffeefbfe828, handler=<unavailable>) at api-arguments-inl.h:158:3 [opt]
    frame #9: 0x00000001005b03c0 node`v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(isolate=0x0000000104f44000, function=<unavailable>, new_target=<unavailable>, fun_data=<unavailable>, receiver=<unavailable>, args=BuiltinArguments @ 0x00007ffeefbfe8e0) at builtins-api.cc:111:36 [opt]
    frame #10: 0x00000001005ae6f3 node`v8::internal::Builtin_Impl_HandleApiCall(args=BuiltinArguments @ 0x00007ffeefbfe910, isolate=0x0000000104f44000) at builtins-api.cc:141:5 [opt]
    frame #11: 0x00000001005ae14c node`v8::internal::Builtin_HandleApiCall(args_length=5, args_object=0x00007ffeefbfea58, isolate=0x0000000104f44000) at builtins-api.cc:129:1 [opt]
    frame #12: 0x00000001013e3060 node`Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit + 64
    frame #13: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #14: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #15: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #16: 0x000000010144d359 node`Builtins_ArrayForEach + 5113
    frame #17: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #18: 0x000000010144d359 node`Builtins_ArrayForEach + 5113
    frame #19: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #20: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #21: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #22: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #23: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #24: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #25: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #26: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #27: 0x0000000101107a55 node`Builtins_InterpreterEntryTrampoline + 949
    frame #28: 0x00000001010f9dfd node`Builtins_JSEntryTrampoline + 93
    frame #29: 0x00000001010f9bd8 node`Builtins_JSEntry + 120
    frame #30: 0x00000001007190b3 node`v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [inlined] v8::internal::GeneratedCode<unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, long, unsigned long**>::Call(this=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>, args=<unavailable>) at simulator.h:136:12 [opt]
    frame #31: 0x00000001007190aa node`v8::internal::(anonymous namespace)::Invoke(isolate=0x0000000104f44000, params=<unavailable>)::InvokeParams const&) at execution.cc:266 [opt]
    frame #32: 0x00000001007189e4 node`v8::internal::Execution::Call(isolate=0x0000000104f44000, callable=<unavailable>, receiver=<unavailable>, argc=2, argv=0x00007ffeefbff488) at execution.cc:358:10 [opt]
    frame #33: 0x00000001004ffd8b node`v8::Function::Call(this=0x000000010603b1a0, context=<unavailable>, recv=<unavailable>, argc=2, argv=0x00007ffeefbff488) at api.cc:4917:7 [opt]
    frame #34: 0x000000010000327c node`node::InternalMakeCallback(env=0x000000010603a000, resource=(val_ = 0x000000010603b020), recv=(val_ = 0x000000010603b020), callback=(val_ = 0x000000010603b320), argc=0, argv=0x0000000000000000, asyncContext=(async_id = 0, trigger_async_id = 0)) at callback.cc:185:20
    frame #35: 0x0000000100003a10 node`node::MakeCallback(isolate=0x0000000104f44000, recv=(val_ = 0x000000010603b020), callback=(val_ = 0x000000010603b320), argc=0, argv=0x0000000000000000, asyncContext=(async_id = 0, trigger_async_id = 0)) at callback.cc:248:7
    frame #36: 0x000000010008c2fc node`node::Environment::CheckImmediate(handle=0x000000010603a198) at env.cc:828:5
    frame #37: 0x00000001010d2ed8 node`uv__run_check(loop=0x0000000102ced620) at loop-watcher.c:67:1
    frame #38: 0x00000001010c7c08 node`uv_run(loop=0x0000000102ced620, mode=UV_RUN_DEFAULT) at core.c:382:5
    frame #39: 0x00000001001f4654 node`node::NodeMainInstance::Run(this=0x00007ffeefbff8d0) at node_main_instance.cc:129:9
    frame #40: 0x00000001000f89f4 node`node::Start(argc=3, argv=0x00007ffeefbffa70) at node.cc:995:38
    frame #41: 0x0000000101a1115e node`main(argc=3, argv=0x00007ffeefbffa70) at node_main.cc:126:10
    frame #42: 0x00007fff7fd5d3d5 libdyld.dylib`start + 1

Flarna commented Aug 14, 2020

Copy link
Copy Markdown
Member Author

I did an experiment and added the commits from this PR on top of 12.18.1. gulp and glob-watcher tests are green then.

So I still don't know the root cause of the failures introduced in 12.18.2 but the changes from this PR seems to be ok.

Flarna commented Aug 14, 2020

Copy link
Copy Markdown
Member Author

The fsevents version (1.2.13) used by glob-watcher users a Nan::AsyncResource. My gut feeling tells that this is related.
Will investigate further and try to get a reproducer on an OS which is easier for me to work on. But this is a separate task then this PR.

This comment has been minimized.

Sign in to view

Flarna commented Aug 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Did some more investigations. Looks like the problem is a missing initialization of this uv_async_t as this may result in this check to return true resulting in not calling uv_async_init().

If I add memset(&async, 0, sizeof(async)) here the crashes are gone (including the segfaults seen before 14.5.0).

nodejs-github-bot commented Aug 14, 2020

Copy link
Copy Markdown
Collaborator

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

Flarna mentioned this pull request Aug 15, 2020
Merged

Qard commented Aug 15, 2020

Copy link
Copy Markdown
Member

The correct thing to do would be to not use the uv_async_t until after it is initialized with uv_async_init(...). The pattern of checking async.data and reinitializing the same object is probably not safe. If it reinitializes while it is still in-use that could cause some chaos too.

Flarna added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 15, 2020

MylesBorins commented Aug 18, 2020

Copy link
Copy Markdown
Contributor

Is this ready to land? How does it differ from what we landed on master / v14.x?

Qard commented Aug 18, 2020

Copy link
Copy Markdown
Member

@MylesBorins It's really just a very small difference in the the patch contents. Functionally they are identical.

Master: https://github.com/nodejs/node/pull/33778/files#diff-d047841e5be2101da57516c10fabb17c
This: https://github.com/nodejs/node/pull/34776/files#diff-d047841e5be2101da57516c10fabb17c

V8 changed to use ScopedExceptionHandler in a block rather than GotoIfException(...) on the next line.

Flarna commented Aug 18, 2020

Copy link
Copy Markdown
Member Author

Yes, it's more or less the same as on master/v14. Backport PR was needed because the patch didn't land clean.

I think it's ready to land.

Qard mentioned this pull request Aug 23, 2020
Closed

Flarna commented Sep 4, 2020

Copy link
Copy Markdown
Member Author

@nodejs/lts Any chance that this gets included into the next v12 release? According to nodejs/Release#494 12.19.0 should be out soon.

Flarna force-pushed the v12-thenables-v8-again branch from ab727f2 to cb58cdb Compare September 15, 2020 22:05
addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020

This comment has been minimized.

Sign in to view
addaleax force-pushed the v12.x-staging branch 3 times, most recently from 55fe022 to 65b7bf4 Compare September 22, 2020 17:57

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

MylesBorins commented Sep 23, 2020

Copy link
Copy Markdown
Contributor

looks like V8 CI also failed on this one

richardlau commented Sep 23, 2020

Copy link
Copy Markdown
Member

looks like V8 CI also failed on this one

Infra blip. V8-CI rerun: https://ci.nodejs.org/job/node-test-commit-v8-linux/3396/

richardlau commented Sep 23, 2020

Copy link
Copy Markdown
Member

looks like V8 CI also failed on this one

Infra blip. V8-CI rerun: https://ci.nodejs.org/job/node-test-commit-v8-linux/3396/

Or maybe not, looks like the job has merge issues checking out this PR.

Flarna commented Sep 23, 2020

Copy link
Copy Markdown
Member Author

I will do a rebase and retry in couple of minutes

Flarna force-pushed the v12-thenables-v8-again branch from cb58cdb to 8befd11 Compare September 23, 2020 18:17
Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020

This comment has been minimized.

Sign in to view

Flarna commented Sep 23, 2020

Copy link
Copy Markdown
Member Author

v8 CI still fails because a lot files I haven't even touched require a merge according to git....

MylesBorins commented Sep 23, 2020

Copy link
Copy Markdown
Contributor

/cc @nodejs/build any idea what's going on with V8 CI?

richardlau commented Sep 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

/cc @nodejs/build any idea what's going on with V8 CI?

For some reason testing this PR was leaving the workspaces in an umerged state that was preventing the checkouts on subsequent jobs from succeeding. I've wiped out the workspaces and restarted the job.
new V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3399/

This comment has been minimized.

Sign in to view
Qard and others added 2 commits September 23, 2020 21:21
deps: V8: cherry-pick eec10a2fd8fa
7182ee4 
Original commit message:

    [promisehook] Add before/after hooks to thenable tasks

    This will allow Node.js to properly track async context in thenables.

    Change-Id: If441423789a78307a57ad7e645daabf551cddb57
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
    Commit-Queue: Gus Caplan <me@gus.host>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2

PR-URL: nodejs#33778
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
test: AsyncLocalStorage works with thenables
227a637 
This adds a test to verify that AsyncLocalStorage works with thenables.

PR-URL: nodejs#34008
Refs: nodejs#33778
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Flarna force-pushed the v12-thenables-v8-again branch from 8befd11 to 227a637 Compare September 23, 2020 19:22
Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020

nodejs-github-bot commented Sep 23, 2020 •
edited by Flarna
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33222/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3400/

Flarna commented Sep 23, 2020

Copy link
Copy Markdown
Member Author

rebased once more as there was a conflict. CI is green now 🎉

addaleax pushed a commit that referenced this pull request Sep 23, 2020
deps: V8: cherry-pick eec10a2fd8fa
5db8b35 
Original commit message:

    [promisehook] Add before/after hooks to thenable tasks

    This will allow Node.js to properly track async context in thenables.

    Change-Id: If441423789a78307a57ad7e645daabf551cddb57
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
    Commit-Queue: Gus Caplan <me@gus.host>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2
Backport-PR-URL: #34776
PR-URL: #33778
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax pushed a commit that referenced this pull request Sep 23, 2020
test: AsyncLocalStorage works with thenables
232f6e1 
This adds a test to verify that AsyncLocalStorage works with thenables.

Backport-PR-URL: #34776
PR-URL: #34008
Refs: #33778
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax commented Sep 23, 2020

Copy link
Copy Markdown
Member

Landed in b7e4d5f...232f6e1 :)

addaleax closed this Sep 23, 2020
Flarna deleted the v12-thenables-v8-again branch September 23, 2020 20:58
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Qard Qard approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL