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.

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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Aug 14, 2020 •
edited
Loading

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 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
Copy link
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
Qard approved these changes Aug 14, 2020
View reviewed changes
Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

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

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

@Flarna
Copy link
Member Author

Flarna commented Aug 14, 2020

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
Copy link
Member Author

Flarna commented Aug 14, 2020

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
Copy link
Member Author

Flarna commented Aug 14, 2020

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.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Flarna
Copy link
Member Author

Flarna commented Aug 14, 2020 •
edited
Loading

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
Copy link
Collaborator

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

@Flarna Flarna mentioned this pull request Aug 15, 2020
Merged
@Qard
Copy link
Member

Qard commented Aug 15, 2020

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 Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 15, 2020
@MylesBorins
Copy link
Contributor

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

@Qard
Copy link
Member

Qard commented Aug 18, 2020

@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
Copy link
Member Author

Flarna commented Aug 18, 2020

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 Qard mentioned this pull request Aug 23, 2020
Closed
@Flarna
Copy link
Member Author

Flarna commented Sep 4, 2020

@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.

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2020
@nodejs-github-bot

This comment has been minimized.

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

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@MylesBorins
Copy link
Contributor

looks like V8 CI also failed on this one

@richardlau
Copy link
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
Copy link
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
Copy link
Member Author

Flarna commented Sep 23, 2020

I will do a rebase and retry in couple of minutes

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020
@github-actions 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

This comment has been minimized.

Sign in to view
@Flarna
Copy link
Member Author

Flarna commented Sep 23, 2020

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

@MylesBorins
Copy link
Contributor

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

@richardlau
Copy link
Member

richardlau commented Sep 23, 2020 •
edited
Loading

/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/

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Qard and others added 2 commits September 23, 2020 21:21
@Qard @Flarna
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 <[email protected]>
    Reviewed-by: Sathya Gunasekaran  <[email protected]>
    Commit-Queue: Gus Caplan <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2

PR-URL: nodejs#33778
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@Flarna
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020
@github-actions 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
Copy link
Collaborator

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

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
Copy link
Member Author

Flarna commented Sep 23, 2020

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

addaleax pushed a commit that referenced this pull request Sep 23, 2020
@Qard @addaleax
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 <[email protected]>
    Reviewed-by: Sathya Gunasekaran  <[email protected]>
    Commit-Queue: Gus Caplan <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2
Backport-PR-URL: #34776
PR-URL: #33778
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 23, 2020
@Flarna @addaleax
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member

Landed in b7e4d5f...232f6e1 :)

@addaleax addaleax closed this Sep 23, 2020
@Flarna Flarna deleted the v12-thenables-v8-again branch September 23, 2020 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qard Qard Qard approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@Flarna @MylesBorins @nodejs-github-bot @Qard @richardlau @addaleax