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: new API - Session.connectToMainThread #28870

Merged
merged 0 commits into from
Sep 16, 2019
Merged

inspector: new API - Session.connectToMainThread #28870

merged 0 commits into from
Sep 16, 2019

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Jul 26, 2019

This API is designed to enable worker threads use Inspector protocol
on main thread (and other workers through NodeWorker domain).

Note that worker can cause dead lock by suspending itself. I will
work on a new API that will allow workers to be hidden from the
inspector.

Fixes: #28828

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

@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 Jul 26, 2019
const callFrameId = (await pausedPromise).params.callFrames[0].callFrameId;

// Delay to ensure main thread is truly suspended
await new Promise((resolve) => setTimeout(resolve, 200));
Copy link
Member

Choose a reason for hiding this comment

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

Why waiting for Debugger.paused is not enough in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sure main thread will wait while this worker is busy (e.g. if the API client decided to do IO)

test/parallel/test-inspector-connect-main-thread.js Outdated Show resolved Hide resolved

// This ensures that worker does not terminate while waiting for
// inspector response
function wrapCallbackToKeepThreadAlive(callback) {
Copy link
Member

Choose a reason for hiding this comment

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

Do all clients of the new API will need something like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be fixed separately.

src/inspector_agent.h Outdated Show resolved Hide resolved
doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/inspector.md Outdated Show resolved Hide resolved
doc/api/inspector.md Outdated Show resolved Hide resolved
a front-end connected to the Inspector WebSocket port.
Connects a session to the inspector back-end.

### session.connect()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### session.connect()
### session.connectToMainThread()

Copy link
Member

Choose a reason for hiding this comment

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

Also, to clarify: This connects to the parent thread and not necessarily the main thread, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, replied to a different thread.

Current implementation does not support "worker trees" - e.g. only workers with the main thread as a parent are reported by the NodeWorker domain. Is workers spawning workers intenteded use-case? Then it will need to be fixed and I expect that the fix will make this API work in a consistent way.

Copy link
Member

Choose a reason for hiding this comment

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

Is workers spawning workers intenteded use-case?

Yes, that’s possible, mostly because there’s no real reason to prevent it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then, I'll try fixing NodeWorker domain. This API will be consistent with what that domain reports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the proposed fix: #28872

@eugeneo eugeneo added the inspector Issues and PRs related to the V8 inspector protocol label Jul 26, 2019
doc/api/errors.md Outdated Show resolved Hide resolved
@eugeneo
Copy link
Contributor Author

eugeneo commented Jul 28, 2019

I added a new commit so the callback is now executed in a micro task so there are less surprises.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jul 29, 2019

The test added here failed in CI in the job that runs all tests in worker threads (using tools/test.py --worker).

@eugeneo
Copy link
Contributor Author

eugeneo commented Jul 29, 2019

This test should only be started on a main thread, I marked it as such.

There are some other relevant test timeouts, I will be looking into them.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 29, 2019

@Trott
Copy link
Member

Trott commented Jul 29, 2019

CI passed. Is this ready for reviews? Or did you want to first look more closely at the timeouts you mentioned?

@Trott
Copy link
Member

Trott commented Jul 31, 2019

@eugeneo CI passed. Did you still want to look more closely at the timeouts you mentioned?

@nodejs/inspector This could use some reviews.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

This LGTM and I'm +1 on the change and also don't have a good intuition about what we should do in the cases discussed (worker spawned worker connecting).

@eugeneo
Copy link
Contributor Author

eugeneo commented Jul 31, 2019

I rebased the change (there were some changes to how the Inspector works with the workers) and squashed individual changes

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Aug 1, 2019

/ping @nodejs/workers

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 1, 2019
@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 1, 2019
a front-end connected to the Inspector WebSocket port.
Connects a session to the inspector back-end.

### session.connectToMainThread()
Copy link
Member

Choose a reason for hiding this comment

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

To clarify again: This does connect to the parent thread and not the main thread, right? In that case the naming should reflect that, imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All workers will connect to a main thread, I added a test case. This is based on #28872 that makes all workers be reported by the main thread.

const common = require('../common');

common.skipIfInspectorDisabled();
common.skipIfWorker();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we skip this if the process is a Worker thread? We should definitely also test what happens if this code is run inside a Worker, if we can. For now, this check breaks the test, because it doesn’t allow running this file in a Worker, not even from the test itself.

If there’s concern about the isMainThread check below, you can use a solution based on environment variables, e.g. the process.env.HAS_STARTED_WORKER bit in test/parallel/test-worker-exit-code.js. That’s a standard way of dealing with it in our test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test will not work in a worker as it expects the code to be ran on the main thread. I updated the check.

return new Promise((resolve) => session.once(notification, resolve));
}

function WaitForWorker() {
Copy link
Member

Choose a reason for hiding this comment

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

We generally use lowerCamelCase for JS functions (ditto for the functions below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

function doConsoleLogIn5ms() {
setTimeout(() => {
console.log('Message for a test');
}, 5);
Copy link
Member

Choose a reason for hiding this comment

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

Is this used somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is for ensureListenerDoesNotInterrupt

if (isMainThread) {
RunTestWorker();
} else {
RunDebugging();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RunDebugging();
runDebugging().then(common.mustCall());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

if (isMainThread) {
RunTestWorker();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RunTestWorker();
runTestWorker().then(common.mustCall());

(Adding “Changes requested” because the test really does not work in its current form)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 4, 2019
@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 6, 2019

@addaleax thank you for the review. I did my best to update the test to better reflect semantics. Please, take another look.

Note that merge commit will go away after the rebase/squash, I did the merge so code review comments are easier to trace.

@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 16, 2019

@addaleax please let me know if there are still any changes that need to be implemented.

@Trott
Copy link
Member

Trott commented Aug 25, 2019

@addaleax Does this look good to you now? Or are there still changes you'd like to see?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks for the ping; yeah, I think everything here has been addressed, I just didn’t have time to fully review this yet.

() => { consoleLogHappened = true; });
parentPort.postMessage({ consoleLogIn10ms: true });
while ((Date.now() - currentTime) < 50) {
// Spin wait for 200ms
Copy link
Member

Choose a reason for hiding this comment

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

The comment and the code seem to mismatch here 🙂

@nodejs-github-bot
Copy link
Collaborator

@eugeneo
Copy link
Contributor Author

eugeneo commented Aug 26, 2019

I fixed the comment and did the rebase - hence the force-push.

@Trott
Copy link
Member

Trott commented Sep 16, 2019

Given that it's a timeout and it's on FreeBSD (which is a bit of a canary in terms of being the first thing to have problems when tests are in parallel that perhaps should not be), perhaps the test should be in sequential? I say that without looking at it though....

@Trott
Copy link
Member

Trott commented Sep 16, 2019

This makes me think it should probably be in sequential:

  // Delay to ensure main thread is truly suspended
  await new Promise((resolve) => setTimeout(resolve, 50));

@nodejs/testing

@Trott
Copy link
Member

Trott commented Sep 16, 2019

Not sure if this is an unrelated issue or not:

$ tools/test.py -j 96 --repeat 192 test/parallel/test-inspector-connect-main-thread.js 
=== release test-inspector-connect-main-thread ===                   
Path: parallel/test-inspector-connect-main-thread
Message NodeWorker.enable was sent
Skip child is done
Message Debugger.enable was sent
Message Runtime.enable was sent
Message Debugger.setBreakpointByUrl was sent
Message for a test
Message Debugger.evaluateOnCallFrame was sent
Message Debugger.resume was sent
Worker is done
out/Release/node[47372]: ../src/node_platform.cc:283:void node::PerIsolatePlatformData::Shutdown(): Assertion `(foreground_tasks_.Pop()) == nullptr' failed.
 1: 0x100078e0c node::Abort() [/Users/trott/io.js/out/Release/node]
 2: 0x100078bb9 node::AppendExceptionLine(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>, node::ErrorHandlingMode) [/Users/trott/io.js/out/Release/node]
 3: 0x1000cd679 node::PerIsolatePlatformData::Shutdown() [/Users/trott/io.js/out/Release/node]
 4: 0x1000cdd04 node::NodePlatform::UnregisterIsolate(v8::Isolate*) [/Users/trott/io.js/out/Release/node]
 5: 0x1000e69e5 node::worker::WorkerThreadData::~WorkerThreadData() [/Users/trott/io.js/out/Release/node]
 6: 0x1000e4e24 node::worker::Worker::Run() [/Users/trott/io.js/out/Release/node]
 7: 0x1000e6cd6 node::worker::Worker::StartThread(v8::FunctionCallbackInfo<v8::Value> const&)::$_2::__invoke(void*) [/Users/trott/io.js/out/Release/node]
 8: 0x7fff7c1002eb _pthread_body [/usr/lib/system/libsystem_pthread.dylib]
 9: 0x7fff7c103249 _pthread_start [/usr/lib/system/libsystem_pthread.dylib]
10: 0x7fff7c0ff40d thread_start [/usr/lib/system/libsystem_pthread.dylib]
Command: out/Release/node /Users/trott/io.js/test/parallel/test-inspector-connect-main-thread.js
--- CRASHED (Signal: 6) ---

@Trott
Copy link
Member

Trott commented Sep 16, 2019

#29582

addaleax added a commit to addaleax/node that referenced this pull request Sep 17, 2019
Using `console.log()` likely interferes with the functionality of the
test, which also checks the interaction between inspector
and `console.log()` as part of the test. Using `process._rawDebug()`
solves that issue.

Refs: nodejs#28870
Refs: nodejs#29582
Trott pushed a commit to Trott/io.js that referenced this pull request Sep 17, 2019
Using `console.log()` likely interferes with the functionality of the
test, which also checks the interaction between inspector
and `console.log()` as part of the test. Using `process._rawDebug()`
solves that issue.

Refs: nodejs#28870
Refs: nodejs#29582

PR-URL: nodejs#29588
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Sep 17, 2019
While V8 itself should not have any remaining tasks on the queue
during platform shutdown, our inspector implementation may do so.
Thus, the checks verifying that no tasks are queued at that point
make some of the inspector tasks flaky.
Remove the checks and replace them by explicitly destroying all
tasks that are left.

Refs: nodejs#25653
Refs: nodejs#28870 (comment)
targos pushed a commit that referenced this pull request Sep 20, 2019
This API is designed to enable worker threads use Inspector protocol
on main thread (and other workers through NodeWorker domain).

Note that worker can cause dead lock by suspending itself. I will
work on a new API that will allow workers to be hidden from the
inspector.

Fixes: #28828
PR-URL: #28870
Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2019
Using `console.log()` likely interferes with the functionality of the
test, which also checks the interaction between inspector
and `console.log()` as part of the test. Using `process._rawDebug()`
solves that issue.

Refs: #28870
Refs: #29582

PR-URL: #29588
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Trott pushed a commit that referenced this pull request Sep 23, 2019
While V8 itself should not have any remaining tasks on the queue
during platform shutdown, our inspector implementation may do so.
Thus, the checks verifying that no tasks are queued at that point
make some of the inspector tasks flaky.
Remove the checks and replace them by explicitly destroying all
tasks that are left.

Refs: #25653
Refs: #28870 (comment)

PR-URL: #29587
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
targos pushed a commit that referenced this pull request Sep 23, 2019
While V8 itself should not have any remaining tasks on the queue
during platform shutdown, our inspector implementation may do so.
Thus, the checks verifying that no tasks are queued at that point
make some of the inspector tasks flaky.
Remove the checks and replace them by explicitly destroying all
tasks that are left.

Refs: #25653
Refs: #28870 (comment)

PR-URL: #29587
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
BridgeAR added a commit that referenced this pull request Sep 24, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
This API is designed to enable worker threads use Inspector protocol
on main thread (and other workers through NodeWorker domain).

Note that worker can cause dead lock by suspending itself. I will
work on a new API that will allow workers to be hidden from the
inspector.

Fixes: #28828
PR-URL: #28870
Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
Using `console.log()` likely interferes with the functionality of the
test, which also checks the interaction between inspector
and `console.log()` as part of the test. Using `process._rawDebug()`
solves that issue.

Refs: #28870
Refs: #29582

PR-URL: #29588
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
While V8 itself should not have any remaining tasks on the queue
during platform shutdown, our inspector implementation may do so.
Thus, the checks verifying that no tasks are queued at that point
make some of the inspector tasks flaky.
Remove the checks and replace them by explicitly destroying all
tasks that are left.

Refs: #25653
Refs: #28870 (comment)

PR-URL: #29587
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
BridgeAR added a commit that referenced this pull request Sep 25, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
BridgeAR added a commit that referenced this pull request Sep 25, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
@theanarkh
Copy link
Contributor

theanarkh commented Jan 13, 2023

@eugeneo Can we use connectToMainThread API to send a Runtime.evaluate command to V8 inspector for running JavaScript code ? Because connectToMainThread is implemented by RequestInterrupt, and this discussion said that JavaScript code should not be called in RequestInterrupt callback.

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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connect inspector.Session to inspector in another process or thread.
9 participants