Revert "n-api: detect deadlocks in thread-safe function"

[addaleax]

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

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

[gabrielschulhof]

napi_would_deadlock is part of at least 14.1.0, so IINM removing it would be semver-major. Shouldn't we instead think of a better way of detecting whether we are on a JS thread?

[gabrielschulhof]

How can you detect whether you are on a JS thread or not without knowing a priori that you are on a JS thread? Is there a way of finding out whether an isolate is active on the current thread without calling v8::Isolate::GetCurrent()?

[addaleax]

napi_would_deadlock is part of at least 14.1.0, so IINM removing it would be semver-major.

@gabrielschulhof Hm right, we should probably leave the enum value in place. I’ll update the PR.

Shouldn't we instead think of a better way of detecting whether we are on a JS thread?

Yes. That hasn’t happened for 11 days since #33276 was opened, hence the revert PR now.

How can you detect whether you are on a JS thread or not without knowing a priori that you are on a JS thread? Is there a way of finding out whether an isolate is active on the current thread without calling v8::Isolate::GetCurrent()?

I wasn’t aware that Isolate::GetCurrent() is invalid, which is why I suggested it, but I don’t really know of any alternatives.

[addaleax]

@gabrielschulhof I’ve added the definition back to the header/the doc, PTAL

(I’m assuming that your objection was about API/ABI compatibility and not the revert of the feature itself?)

@addaleax I'm not too fond of the revert either, because I'm frustrated by the fact that we have no way of detecting that we are on a JS thread, but I understand the reason for it. I guess, with the enum value in place we have secured the option of eventually actually, possibly returning that value, once we've figured out how to properly detect that we are on a JS thread.

[gabrielschulhof]

If we replace the comments in the documentation instead of removing the entirely, we'll at least have some sort of fix for the original issue.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[gabrielschulhof]

@addaleax question before this lands: Can we use node::Environment::GetThreadLocalEnv() to determine whether we are on a JS thread?

[gabrielschulhof]

I mean, instead of v8::Isolate::GetCurrent()?

[gabrielschulhof]

@addaleax like so (still running the tests locally, so no PR yet).

[addaleax]

@gabrielschulhof Currently yes, but we should get rid of that API, and it won’t work when an Environment/Isolate is moved between threads. I don’t think that would be a good idea.

Ultimately, this is a programmer error and there are tools that help diagnose this kind of situation. If we can’t come up with a good solution that doesn’t yield false positives, we should let this be.

@addaleax gotcha! Thanks for clarifying!

[BridgeAR]

Landed in b18d8dd