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

inspector: turn platform tasks that outlive Agent into no-ops #30031

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 18, 2019

Turn tasks scheduled on the v8::Isolate or on the given platform
into no-ops if the underlying MainThreadInterface has gone away
before the task could be run (which would happen when the
Environment instance and with it the inspector::Agent instance
are destroyed).

This addresses an issue that Electron has been having with
inspector support, and generally just seems like the right thing
to do, as we may not fully be in control of the relative timing
of Environment teardown, platform tasks execution, and the
execution of RequestInterrupt() callbacks (although
the former two always happen in the same order in our own code).

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

Turn tasks scheduled on the `v8::Isolate` or on the given platform
into no-ops if the underlying `MainThreadInterface` has gone away
before the task could be run (which would happen when the
`Environment` instance and with it the `inspector::Agent` instance
are destroyed).

This addresses an issue that Electron has been having with
inspector support, and generally just seems like the right thing
to do, as we may not fully be in control of the relative timing
of Environment teardown, platform tasksexecution, and the
execution of `RequestInterrupt()` callbacks (although
the former two always happen in the same order in our own code).
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Oct 18, 2019
@nodejs-github-bot
Copy link
Collaborator

@eugeneo
Copy link
Contributor

eugeneo commented Oct 18, 2019

I’m reviewing from the phone so I am not sure if I captured the details of the change. It looks like this is the scenario where MainThreadHeandle is supposed to be used... I will try to do a better review this weekend.

@addaleax
Copy link
Member Author

@eugeneo Take your time :)

I’m not sure how MainThreadHandle would be usable here, though.

Copy link
Contributor

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this!

src/inspector/main_thread_interface.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

lgtm :)

@nodejs-github-bot
Copy link
Collaborator

src/inspector/main_thread_interface.cc Outdated Show resolved Hide resolved
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 22, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax added a commit that referenced this pull request Oct 23, 2019
Turn tasks scheduled on the `v8::Isolate` or on the given platform
into no-ops if the underlying `MainThreadInterface` has gone away
before the task could be run (which would happen when the
`Environment` instance and with it the `inspector::Agent` instance
are destroyed).

This addresses an issue that Electron has been having with
inspector support, and generally just seems like the right thing
to do, as we may not fully be in control of the relative timing
of Environment teardown, platform tasksexecution, and the
execution of `RequestInterrupt()` callbacks (although
the former two always happen in the same order in our own code).

PR-URL: #30031
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@addaleax
Copy link
Member Author

Landed in 7a82e5e

@addaleax addaleax closed this Oct 23, 2019
@addaleax addaleax deleted the inspector-task-lifetime branch October 23, 2019 21:14
targos pushed a commit that referenced this pull request Oct 24, 2019
Turn tasks scheduled on the `v8::Isolate` or on the given platform
into no-ops if the underlying `MainThreadInterface` has gone away
before the task could be run (which would happen when the
`Environment` instance and with it the `inspector::Agent` instance
are destroyed).

This addresses an issue that Electron has been having with
inspector support, and generally just seems like the right thing
to do, as we may not fully be in control of the relative timing
of Environment teardown, platform tasksexecution, and the
execution of `RequestInterrupt()` callbacks (although
the former two always happen in the same order in our own code).

PR-URL: #30031
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@targos targos mentioned this pull request Nov 5, 2019
targos pushed a commit that referenced this pull request Nov 8, 2019
Turn tasks scheduled on the `v8::Isolate` or on the given platform
into no-ops if the underlying `MainThreadInterface` has gone away
before the task could be run (which would happen when the
`Environment` instance and with it the `inspector::Agent` instance
are destroyed).

This addresses an issue that Electron has been having with
inspector support, and generally just seems like the right thing
to do, as we may not fully be in control of the relative timing
of Environment teardown, platform tasksexecution, and the
execution of `RequestInterrupt()` callbacks (although
the former two always happen in the same order in our own code).

PR-URL: #30031
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
Turn tasks scheduled on the `v8::Isolate` or on the given platform
into no-ops if the underlying `MainThreadInterface` has gone away
before the task could be run (which would happen when the
`Environment` instance and with it the `inspector::Agent` instance
are destroyed).

This addresses an issue that Electron has been having with
inspector support, and generally just seems like the right thing
to do, as we may not fully be in control of the relative timing
of Environment teardown, platform tasksexecution, and the
execution of `RequestInterrupt()` callbacks (although
the former two always happen in the same order in our own code).

PR-URL: #30031
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2019
Turn tasks scheduled on the `v8::Isolate` or on the given platform
into no-ops if the underlying `MainThreadInterface` has gone away
before the task could be run (which would happen when the
`Environment` instance and with it the `inspector::Agent` instance
are destroyed).

This addresses an issue that Electron has been having with
inspector support, and generally just seems like the right thing
to do, as we may not fully be in control of the relative timing
of Environment teardown, platform tasksexecution, and the
execution of `RequestInterrupt()` callbacks (although
the former two always happen in the same order in our own code).

PR-URL: #30031
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants