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

test: skip test_threadsafe_function for debug build #33276

Closed
wants to merge 1 commit into from
Closed

test: skip test_threadsafe_function for debug build #33276

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 7, 2020

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@danbev
test: skip test_threadsafe_function for debug build
5aa511e 
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.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 7, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@danbev danbev added the node-api Issues and PRs related to the Node-API. label May 7, 2020
@danbev danbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2020
@danbev
Copy link
Contributor Author

danbev commented May 12, 2020

@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

@mhdawson
Copy link
Member

@gabrielschulhof do you have an opinion on this?

mhdawson
mhdawson approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, I think its ok if this is disabled for debug only as I don't see a good work around.

@addaleax
Copy link
Member

Uh … idk, I’m sorry I suggested this in #32860, I was not aware that this was not valid V8 API usage. I’d probably prefer to revert #32860 then …

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 12, 2020
@addaleax
Copy link
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?)

@richardlau richardlau mentioned this pull request May 12, 2020
Merged
@danbev
Copy link
Contributor Author

danbev commented May 13, 2020

Uh … idk, I’m sorry I suggested this in #32860, I was not aware that this was not valid V8 API usage. I’d probably prefer to revert #32860 then …

No problems. We can close this and revert #32860 instead.

@mhdawson
Copy link
Member

@addaleax I'm sure addons are tested, just not in debug mode.

@mhdawson mhdawson mentioned this pull request May 13, 2020
Closed
@addaleax
Copy link
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 addaleax mentioned this pull request May 14, 2020
Closed
addaleax added a commit to addaleax/node that referenced this pull request May 18, 2020
@addaleax
Revert "n-api: detect deadlocks in thread-safe function"
9eec1ed 
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
@addaleax addaleax mentioned this pull request May 18, 2020
Closed
3 tasks
@addaleax
Copy link
Member

Opened #33453 with a revert PR

@danbev
Copy link
Contributor Author

danbev commented May 18, 2020

Closing in favor of #33453. @addaleax Thanks!

@danbev danbev closed this May 18, 2020
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 23, 2020
@addaleax @BridgeAR
Revert "n-api: detect deadlocks in thread-safe function"
b18d8dd 
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
@addaleax @codebytere
Revert "n-api: detect deadlocks in thread-safe function"
b4da5bc 
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
@addaleax @codebytere
Revert "n-api: detect deadlocks in thread-safe function"
4c235b0 
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
@addaleax @codebytere
Revert "n-api: detect deadlocks in thread-safe function"
f4cfe94 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengjiawen gengjiawen gengjiawen approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@danbev @nodejs-github-bot @mhdawson @addaleax @gengjiawen