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.

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

investigate flaky test_make_callback/test-async-hooks-gcable in CI #30648

Closed
gireeshpunathil opened this issue Nov 26, 2019 · 8 comments
Closed
Assignees
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. v8 engine Issues and PRs related to the V8 dependency.

Comments

@gireeshpunathil
Copy link
Member

  • Version: master
  • Platform: linux-containerized-sharedlibs
  • Subsystem: async-hooks
11:13:36 not ok 2592 node-api/test_make_callback/test-async-hooks-gcable
11:13:36   ---
11:13:36   duration_ms: 0.332
11:13:36   severity: crashed
11:13:36   exitcode: -11
11:13:36   stack: |-
11:13:36   ...

ref: https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_zlib_x64/16212/consoleFull

@gireeshpunathil
Copy link
Member Author

again:

11:26:27 not ok 2592 node-api/test_make_callback/test-async-hooks-gcable
11:26:27   ---
11:26:27   duration_ms: 0.264
11:26:27   severity: crashed
11:26:27   exitcode: -11
11:26:27   stack: |-
11:26:27   ...

ref: https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_shared_x64/16214/consoleFull

@Trott
Copy link
Member

Trott commented Nov 26, 2019

Yeah, this one is out of control today. Always in the containered builds.

@gireeshpunathil
Copy link
Member Author

more instances: https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_shared_x64/16224/consoleFull

I guess most of the recent CI runs have this affected. Pinning to catch attention.

@gireeshpunathil gireeshpunathil pinned this issue Nov 26, 2019
@gireeshpunathil
Copy link
Member Author

cc @nodejs/v8 @nodejs/async_hooks @nodejs/n-api

@addaleax
Copy link
Member

@gireeshpunathil @Trott This is likely impossible to debug without core dumps and without access to the machine in question? I’ll open an access request on nodejs/build.

@addaleax
Copy link
Member

(gdb) bt
#0  0x000056076b17e854 in v8::internal::ConcurrentMarkingVisitor::VisitPointers(v8::internal::HeapObject, v8::internal::FullObjectSlot, v8::internal::FullObjectSlot) ()
#1  0x000056076b18894c in v8::internal::ConcurrentMarking::Run(int, v8::internal::ConcurrentMarking::TaskState*) ()
#2  0x000056076b0f3b86 in non-virtual thunk to v8::internal::CancelableTask::Run() ()
#3  0x000056076aec9e15 in node::(anonymous namespace)::PlatformWorkerThread(void*) ()
#4  0x00007f8c559dd6db in start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0
#5  0x00007f8c5570688f in clone () from /lib/x86_64-linux-gnu/libc.so.6

This might be related/the same bug as #30498?

@addaleax addaleax added the v8 engine Issues and PRs related to the V8 dependency. label Nov 26, 2019
@addaleax addaleax self-assigned this Nov 27, 2019
@addaleax
Copy link
Member

Okay, results from debugging so far:

  • This is weirdly related to ESM in some way: It always crashes when a major GC happens during our Module::CreateSyntheticModule() calls during bootstrap. I think 796f3d0 might actually be the commit that started these issues.
  • This makes it reproduce somewhat frequently (~ 50 % of the time) locally for me:
diff --git a/deps/v8/src/objects/objects.cc b/deps/v8/src/objects/objects.cc
index 227cff8da47a..c1d344c304a0 100644
--- a/deps/v8/src/objects/objects.cc
+++ b/deps/v8/src/objects/objects.cc
@@ -6458,6 +6458,10 @@ Handle<Derived> HashTable<Derived, Shape>::NewInternal(
   Factory* factory = isolate->factory();
   int length = EntryToIndex(capacity);
   RootIndex map_root_index = Shape::GetMapRootIndex();
+  if (Shape::kEntrySize == 2) {
+    isolate->heap()->CollectAllGarbage(
+        Heap::kNoGCFlags, GarbageCollectionReason::kFullHashtable);
+  }
   Handle<FixedArray> array =
       factory->NewFixedArrayWithMap(map_root_index, length, allocation);
   Handle<Derived> table = Handle<Derived>::cast(array);

That might be a good starting point for debugging in V8 here, and/or bisecting V8. (But that’s for tomorrow rather than today :))

@nodejs/v8

This might be related/the same bug as #30498?

I’m very confident that this is indeed the case now.

@addaleax
Copy link
Member

Here’s a V8 CL that resolves this issue both locally and on the container host: https://chromium-review.googlesource.com/c/v8/v8/+/1939752

addaleax added a commit to addaleax/node that referenced this issue Nov 28, 2019
Original commit message:

    [heap] Ensure SyntheticModule is initialized before next allocation

    Ensure that all fields of `SyntheticModule` are set before creating
    the exports hash table for it, because the latter may trigger
    garbage collection, leading to crashes.

    This has been causing failures in the Node.js CI over the last weeks,
    after making the creating of synthetic modules part of Node’s
    startup sequence.

    (I am generally not very familiar with this part of the V8
    code and there might be a better way, or possibly a way to add a
    reliable regression test, that I am not aware of.)

    Refs: nodejs#30498
    Refs: nodejs#30648
    Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65247}

Refs: v8/v8@ca5b0ec
Fixes: nodejs#30498
Fixes: nodejs#30648
@addaleax addaleax unpinned this issue Nov 29, 2019
uuhan pushed a commit to uuhan/v8 that referenced this issue Nov 29, 2019
Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: nodejs/node#30498
Refs: nodejs/node#30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <[email protected]>
Reviewed-by: Ulan Degenbaev <[email protected]>
Cr-Commit-Position: refs/heads/master@{#65247}
addaleax added a commit that referenced this issue Nov 30, 2019
Original commit message:

[heap] Ensure SyntheticModule is initialized before next allocation

Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: #30498
Refs: #30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <[email protected]>
Reviewed-by: Ulan Degenbaev <[email protected]>
Cr-Commit-Position: refs/heads/master@{#65247}

Refs: https://github.com/v8/v8/commit/ \
ca5b0ec2722d2af4551c01ca78921fa16a26ae72
Fixes: #30498
Fixes: #30648

PR-URL: #30708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 12, 2020
Original commit message:

[heap] Ensure SyntheticModule is initialized before next allocation

Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: #30498
Refs: #30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <[email protected]>
Reviewed-by: Ulan Degenbaev <[email protected]>
Cr-Commit-Position: refs/heads/master@{#65247}

Refs: https://github.com/v8/v8/commit/ \
ca5b0ec2722d2af4551c01ca78921fa16a26ae72
Fixes: #30498
Fixes: #30648

PR-URL: #30708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
Original commit message:

[heap] Ensure SyntheticModule is initialized before next allocation

Ensure that all fields of `SyntheticModule` are set before creating
the exports hash table for it, because the latter may trigger
garbage collection, leading to crashes.

This has been causing failures in the Node.js CI over the last weeks,
after making the creating of synthetic modules part of Node’s
startup sequence.

(I am generally not very familiar with this part of the V8
code and there might be a better way, or possibly a way to add a
reliable regression test, that I am not aware of.)

Refs: #30498
Refs: #30648
Change-Id: I32da4b7bd888c6ec1421f34f5bd52e7bad154c1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1939752
Commit-Queue: Ulan Degenbaev <[email protected]>
Reviewed-by: Ulan Degenbaev <[email protected]>
Cr-Commit-Position: refs/heads/master@{#65247}

Refs: https://github.com/v8/v8/commit/ \
ca5b0ec2722d2af4551c01ca78921fa16a26ae72
Fixes: #30498
Fixes: #30648

PR-URL: #30708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants