test: skip test_threadsafe_function for debug build#33276
Closed
danbev wants to merge 1 commit intonodejs:masterfrom
Closed
test: skip test_threadsafe_function for debug build#33276danbev wants to merge 1 commit intonodejs:masterfrom
danbev wants to merge 1 commit intonodejs:masterfrom
Conversation
This comit suggests skipping this test when the build type is Debug. The
reason for this is that it currently aborts with the following message:
1: 0xe5ba88 node::DumpBacktrace(_IO_FILE*) [out/Debug/node]
2: 0xfc4e2d [out/Debug/node]
3: 0xfc4e4d [out/Debug/node]
4: 0x288c9e8 V8_Fatal(char const*, int, char const*, ...)
[out/Debug/node]
5: 0x288ca13 [out/Debug/node]
6: 0x115ef49 v8::internal::Isolate::Current() [out/Debug/node]
7: 0xec6e1c [out/Debug/node]
8: 0xec9c26 napi_call_threadsafe_function [out/Debug/node]
9: 0x7f3a39de3474
[test/node-api/test_threadsafe_function/build/Debug/binding.node]
10: 0x7f3a39a5a4e2 [/lib64/libpthread.so.0]
11: 0x7f3a399896d3 clone [/lib64/libc.so.6]
Illegal instruction (core dumped)
This is generated from ThreadSafeFunction::Push and the call to
v8::Isolate::GetCurrent() which has a debug mode check (DCHECK) which is
causing this to abort if the current thread's Isolate is a null. As far
as I know there is nothing like v8::internal::Isolate::TryGetCurrent()
exposed with could be used which is the reason for suggesting this test
just be skipped.
Collaborator
gengjiawen
approved these changes
May 7, 2020
danbev
added
the
author ready
Contributor
Author
|
@nodejs/n-api Would someone from the team be able to take a look at this and approve if they think this change is resonable? Thanks |
Member
|
@gabrielschulhof do you have an opinion on this? |
mhdawson
approved these changes
May 12, 2020
Member
mhdawson
left a comment
mhdawson
left a comment
There was a problem hiding this comment.
LGTM, I think its ok if this is disabled for debug only as I don't see a good work around.
Member
addaleax
removed
the
author ready
Member
|
(I don’t want to block this but this should eventually be reverted, so as far as I am concerned this can be landed this as long as an issue or sth equivalent opened about it. Also, I’m surprised that addons still aren’t tested in CI?) |
Contributor
Author
Member
|
@addaleax I'm sure addons are tested, just not in debug mode. |
Member
|
@mhdawson Yes, that was the point. Addon tests are among the set of tests that should definitely run under debug mode in CI. |
addaleax
added a commit
to addaleax/node
that referenced
this pull request
May 18, 2020
This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: nodejs#33276 Refs: nodejs#32860
3 tasks
Member
|
Opened #33453 with a revert PR |
Contributor
Author
BridgeAR
pushed a commit
to BridgeAR/node
that referenced
this pull request
May 23, 2020
This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: nodejs#33276 Refs: nodejs#32860 PR-URL: nodejs#33453 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
codebytere
pushed a commit
that referenced
this pull request
Jun 18, 2020
This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: #33276 Refs: #32860 PR-URL: #33453 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
codebytere
pushed a commit
that referenced
this pull request
Jun 18, 2020
This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: #33276 Refs: #32860 PR-URL: #33453 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
codebytere
pushed a commit
that referenced
this pull request
Jun 30, 2020
This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: #33276 Refs: #32860 PR-URL: #33453 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This comit suggests skipping this test when the build type is
Debug. Thereason for this is that it currently aborts with the following message:
This is generated from
ThreadSafeFunction::Pushand the call tov8::Isolate::GetCurrent()which has a debug mode check (DCHECK) which iscausing this to abort if the current thread's Isolate is a null. As far
as I know there is nothing like v8::internal::Isolate::TryGetCurrent()
exposed with could be used which is the reason for suggesting this test
just be skipped.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes