-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Flaky test sequential/test-http2-settings-flood #31107
Comments
I have tested the above on AIX
Edit: |
Gave access, permission already granted in nodejs/build#1706 |
@miladfarca Can you figure out why it is crashing? |
@addaleax Yes I will be looking into it, might be related to V8 deoptimization |
This issue doesn't seem to be specific to PPC or AIX. It is also failing on x64-freebsd on the CI: https://ci.nodejs.org/job/node-test-commit-freebsd/nodes=freebsd11-x64/30698/console I have done a debug compile on both
There seems to be an issue with V8 de-optimizer. |
@miladfarca: good job! would you mind creating a V8 ticket? |
@ronag I've created this ticket but not sure if we need to reproduce it under V8 (and not nodejs), will wait for their reply: |
@miladfarca Same results with a debug build on MacOS Mojave: $ tools/test.py --mode=debug --repeat 1000 test/sequential/test-http2-ping-flood.js
=== debug test-http2-ping-flood ===
Path: sequential/test-http2-ping-flood
#
# Fatal error in ../deps/v8/src/deoptimizer/deoptimizer.cc, line 253
# Debug check failed: code == topmost_ implies safe_to_deopt_.
#
#
# |
Thanks @Trott , just to confirm your mac is x64 correct? I'll add to the V8 ticket. |
Yes, x64. |
ping @nodejs/v8 |
It appears this is already known to be platform agnostic, but just for additional info, it also has been observed on debian9-docker-armv7 (https://ci.nodejs.org/job/node-test-commit-arm/28686/) |
FWIW this is also occurring in Ubuntu 18.04 containers. |
@addaleax V8 ticket (https://bugs.chromium.org/p/v8/issues/detail?id=10101) is now closed with a comment suggesting an API misuse in node. Issue might be related to this commit/line which might need to be refactored: 7e6104f#diff-9e96b9359319a317a5a1ee45f11eea67R126 |
@miladfarca Thanks for the info, I’ll try to come up with a fix. They are right here, but I am surprised that
I’d like to avoid that, and instead try to not perform the invalid V8 call here. |
It looks like it’s virtually impossible at this point to create “fake” backing stores for objects that don’t fully own their memory allocations, like the sub-field `js_fields_` of `Http2Session`. In particular, it turns out that an `ArrayBuffer` cannot always be easily separated from its backing store in that situation through by detaching it. This commit gives the JS-exposed parts of the class its own memory allocation and its own backing store, simplifying the code a bit and fixing flakiness coming from it, at the cost of one additional layer of indirection when accessing the data. Refs: nodejs#30782 Fixes: nodejs#31107
The test in
sequential/test-http2-settings-flood
sometimes fails on AIX PPC64 withSIGILL
and no other output (about 4/100 times). Bisecting points to @ronag’s cd6b00d, which obviously shouldn’t be causing that issue. It doesn’t reproduce when running the test with--jitless
, so my best guess would be that it’s a V8 issue on PPC.I can’t really debug this any further, the issue doesn’t seem to reproduce under gdb and core dumps don’t seem helpful, so it would be great if somebody in @nodejs/platform-aix (@miladfarca?) could take a look. In the short term, I guess we could also revert the commit above, although that’s obviously not ideal.
The text was updated successfully, but these errors were encountered: