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

src: use env’s instead of isolate’s RequestInterrupt() + v8::Platform #32523

Closed
wants to merge 7 commits into from

Conversation

addaleax
Copy link
Member

src: make Environment::interrupt_data_ atomic

Otherwise this potentially leads to race conditions when used in a
multi-threaded environment (i.e. when used for its very purpose).

src: fix cleanup hook removal for InspectorTimer

Fix this to account for the fact that Stop() may already have been
called from a cleanup hook when the inspector::Agent is deleted
along with the Environment, at which point cleanup hooks are no
longer available.

src: use env->RequestInterrupt() for inspector io thread start

This cleans up Agent::RequestIoThreadStart() significantly.

src: use env->RequestInterrupt() for inspector MainThreadInterface

This simplifies the code significantly, and removes the dependency of
the inspector code on the availability of a MultiIsolatePlatform
(by removing the dependency on a platform altogether).

It also fixes a memory leak that occurs when RequestInterrupt()
is used, but the interrupt handler is never called before the Isolate
is destroyed.

One downside is that this leads to a slight change in timing, because
inspector messages are not dispatched in a re-entrant way. This means
having to harden one test to account for that possibility by making
sure that the stack is always clear through a setImmediate().
This does not affect the assertion made by the test, which is that
messages will not be delivered synchronously while other code is
executing.

#32415

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Otherwise this potentially leads to race conditions when used in a
multi-threaded environment (i.e. when used for its very purpose).
Fix this to account for the fact that `Stop()` may already have been
called from a cleanup hook when the `inspector::Agent` is deleted
along with the `Environment`, at which point cleanup hooks are no
longer available.
This cleans up `Agent::RequestIoThreadStart()` significantly.
This simplifies the code significantly, and removes the dependency of
the inspector code on the availability of a `MultiIsolatePlatform`
(by removing the dependency on a platform altogether).

It also fixes a memory leak that occurs when `RequestInterrupt()`
is used, but the interrupt handler is never called before the Isolate
is destroyed.

One downside is that this leads to a slight change in timing, because
inspector messages are not dispatched in a re-entrant way. This means
having to harden one test to account for that possibility by making
sure that the stack is always clear through a `setImmediate()`.
This does not affect the assertion made by the test, which is that
messages will not be delivered synchronously while other code is
executing.

nodejs#32415
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 27, 2020
@@ -84,7 +84,7 @@ class JSBindingsConnection : public AsyncWrap {

private:
Environment* env_;
JSBindingsConnection* connection_;
BaseObjectPtr<JSBindingsConnection> connection_;
Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, I’m not sure whether this is necessary anymore, but it was at one point while working on this, and it doesn’t hurt for the code to be a bit more robust here.

@addaleax addaleax added the inspector Issues and PRs related to the V8 inspector protocol label Mar 27, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

LGTM, but someone more familiar with inspector code should review it as well.

Comment on lines +742 to +747
Environment** interrupt_data = new Environment*(this);
Environment** dummy = nullptr;
if (!interrupt_data_.compare_exchange_strong(dummy, interrupt_data)) {
delete interrupt_data;
return; // Already scheduled.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's worth adding a comment here explaining what this code does.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mmarchini Done, PTAL :)

if (!interrupt_data_.compare_exchange_strong(dummy, interrupt_data)) {
delete interrupt_data;
return; // Already scheduled.
}

isolate()->RequestInterrupt([](Isolate* isolate, void* data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember if this callback runs asynchronously. Any chance we could get a situation where we request the interrupt here but don't run it before the Isolate is deleted? If that's possible, we would have a leak on interrupt_data.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right – that is a possibility, but I don’t quite know what to do about it (without changing V8 APIs). The leak was already there before, too, and it’s limited to the size of a pointer…

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I have an idea. Let me see. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. If ASAN starts to complain, we can force it to ignore this pointer (for now ASAN is happy, so we can leave it as is).

Copy link
Member Author

Choose a reason for hiding this comment

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

@mmarchini 7e75cc1 should address this by flushing interrupts when freeing the Environment instance … feels a bit like a hack to me but I think it’s the best we can do :)

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 28, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30145/ (:heavy_check_mark:)

@addaleax addaleax added the review wanted PRs that need reviews. label Apr 2, 2020
@addaleax
Copy link
Member Author

addaleax commented Apr 8, 2020

ping @mmarchini

addaleax added a commit that referenced this pull request Apr 10, 2020
Otherwise this potentially leads to race conditions when used in a
multi-threaded environment (i.e. when used for its very purpose).

PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Apr 10, 2020
Fix this to account for the fact that `Stop()` may already have been
called from a cleanup hook when the `inspector::Agent` is deleted
along with the `Environment`, at which point cleanup hooks are no
longer available.

PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Apr 10, 2020
This cleans up `Agent::RequestIoThreadStart()` significantly.

PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Apr 10, 2020
This simplifies the code significantly, and removes the dependency of
the inspector code on the availability of a `MultiIsolatePlatform`
(by removing the dependency on a platform altogether).

It also fixes a memory leak that occurs when `RequestInterrupt()`
is used, but the interrupt handler is never called before the Isolate
is destroyed.

One downside is that this leads to a slight change in timing, because
inspector messages are not dispatched in a re-entrant way. This means
having to harden one test to account for that possibility by making
sure that the stack is always clear through a `setImmediate()`.
This does not affect the assertion made by the test, which is that
messages will not be delivered synchronously while other code is
executing.

#32415

PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Apr 10, 2020
This avoids an edge-case memory leak.

Refs: #32523 (comment)

PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member Author

addaleax commented Apr 10, 2020

Landed in d0a3bf1...a173473

@addaleax addaleax closed this Apr 10, 2020
@addaleax addaleax deleted the inspector-requestinterrupt branch April 10, 2020 16:08
addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020
Otherwise this potentially leads to race conditions when used in a
multi-threaded environment (i.e. when used for its very purpose).

PR-URL: nodejs#32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020
Fix this to account for the fact that `Stop()` may already have been
called from a cleanup hook when the `inspector::Agent` is deleted
along with the `Environment`, at which point cleanup hooks are no
longer available.

PR-URL: nodejs#32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020
This cleans up `Agent::RequestIoThreadStart()` significantly.

PR-URL: nodejs#32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020
This simplifies the code significantly, and removes the dependency of
the inspector code on the availability of a `MultiIsolatePlatform`
(by removing the dependency on a platform altogether).

It also fixes a memory leak that occurs when `RequestInterrupt()`
is used, but the interrupt handler is never called before the Isolate
is destroyed.

One downside is that this leads to a slight change in timing, because
inspector messages are not dispatched in a re-entrant way. This means
having to harden one test to account for that possibility by making
sure that the stack is always clear through a `setImmediate()`.
This does not affect the assertion made by the test, which is that
messages will not be delivered synchronously while other code is
executing.

nodejs#32415

PR-URL: nodejs#32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020
This avoids an edge-case memory leak.

Refs: nodejs#32523 (comment)

PR-URL: nodejs#32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020
This ensures that microtasks scheduled by native immediates are run
after the tasks are done. In particular, this affects the inspector
integration since 6f9f546.

Fixes: nodejs#33002
Refs: nodejs#32523

PR-URL: nodejs#34366
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Sep 23, 2020
Otherwise this potentially leads to race conditions when used in a
multi-threaded environment (i.e. when used for its very purpose).

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Sep 23, 2020
Fix this to account for the fact that `Stop()` may already have been
called from a cleanup hook when the `inspector::Agent` is deleted
along with the `Environment`, at which point cleanup hooks are no
longer available.

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Sep 23, 2020
This cleans up `Agent::RequestIoThreadStart()` significantly.

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Sep 23, 2020
This simplifies the code significantly, and removes the dependency of
the inspector code on the availability of a `MultiIsolatePlatform`
(by removing the dependency on a platform altogether).

It also fixes a memory leak that occurs when `RequestInterrupt()`
is used, but the interrupt handler is never called before the Isolate
is destroyed.

One downside is that this leads to a slight change in timing, because
inspector messages are not dispatched in a re-entrant way. This means
having to harden one test to account for that possibility by making
sure that the stack is always clear through a `setImmediate()`.
This does not affect the assertion made by the test, which is that
messages will not be delivered synchronously while other code is
executing.

#32415

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Sep 23, 2020
This avoids an edge-case memory leak.

Refs: #32523 (comment)

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Sep 23, 2020
This ensures that microtasks scheduled by native immediates are run
after the tasks are done. In particular, this affects the inspector
integration since 6f9f546.

Fixes: #33002
Refs: #32523

Backport-PR-URL: #35241
PR-URL: #34366
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants