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

DRAFT: Add experimental inspector support #2696

Draft
wants to merge 15 commits into
base: master
from

Conversation

@mtharrison
Copy link
Contributor

commented Jul 29, 2019

I wanted to share my progress so far with adding inspector support to Deno via a draft PR. There are still many issues to be ironed out but it feels most productive at this point to get some comments on the approach and suggestions for improvement rather I keep hacking on it in private.

I've added TODO comments to areas I know need work but I'm sure there's lots of other stuff too that I've missed. Some parts are kinda duct-tapey right now but I'm sure they would be improved by some smart architectural decision that eluded me :)

To test this out:

cargo build
./target/debug/deno --inspect-brk run script.js
// Inspect via Chrome Devtools
@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@mtharrison AFAIK "draft PR" won't run CI properly, so you might want to consider making it a regular PR and adding WIP prefix to title ;)

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@bartlomieju I could do, however are there any benefits other than seeing test results? Right now it's quite a ways off that really being relevant as I've not written any tests and this likely breaks a bunch of stuff. This is mainly open now to get feedback on the shape of the feature. WDYT?

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

@mtharrison sure, that was just a suggestion :)

).arg(
Arg::with_name("inspect-brk")
.long("inspect-brk")
.help("Enable inspector and pause on first statement")

This comment has been minimized.

Copy link
@ry

ry Jul 31, 2019

Collaborator

I think --debug is most reasonable. I'd like to avoid having two flags...

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

Two flags are required for two different problems:
--inspect starts the process and allow the inspector to connect any time later, e.g. some tool would like to attach to a running program from time to time and capture one of the heap profilers to build aggregated report later,
--inspect-brk is for the clients which need to set up something ahead of time, e.g. set breakpoints or start profiler. In this mode inspected process should wait until inspector client is connected and sends Runtime.runIfWaitingForDebugger before run. At the same time, I am not sure that pause at first statement is an important part of this behavior.

If I would need to choose only one flag that covers most use cases, I will add --inspect-brk flag that waits for connected client and Runtime.runIfWaitingForDebugger protocol method without breakpoint at first statement.
DevTools or other clients can set required breakpoint manually - it should be possible to fetch entry file name after client connected and before it sent Runtime.runIfWaitingForDebugger

This comment has been minimized.

Copy link
@eugeneo

eugeneo Jul 31, 2019

Pause at first statement is essential for dedicated debugger frontends (e.g. the ones that only access source files from the debug target).

E.g. if you try to debug a short Node.js application using Chrome DevTools you would not be able to set a breakpoint as the application will run to a completion before you get a chance to interrupt it and set a breakpoint.

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

I start the app with --inspect-wait-for-debugger, this flag waits until the client is connected and Runtime.runIfWaitingForDebugger is called.
My frontend connects to the app, calls something to fetch entry file name, e.g. Runtime.evaluate({expression: 'process.entryFileName'}) after I send Debugger.setBreakpointByUrl and only after I prepared required breakpoint frontend calls Runtime.runIfWaitingForDebugger.

In this case, my frontend controls what and where breakpoints are set.

If we would like to have some magic for existing frontends then I prefer to have two separate flags: --inspect-wait-for-debugger and --inspect-set-breakpoint-at-first-stmt. Since we have some clients who do not need this breakpoint at first statement but would like to run some protocol command, e.g., HeapProfiler.start, before Runtime.runIfWaitingForDebugger.

@ry ry requested a review from piscisaureus Jul 31, 2019

let client = Client::new(state, socket);
client.on_connection()
}))
}));

This comment has been minimized.

Copy link
@ry

ry Jul 31, 2019

Collaborator

Wow, warp looks nice.

user_ws_tx = user_ws_tx.send(Message::text(msg)).wait().unwrap();
}
}
});

This comment has been minimized.

Copy link
@ry

ry Jul 31, 2019

Collaborator

TODO(mtharrison): This is a mess. There must be a better way to do this - maybe use async channels or wrap them with a stream?

Actually I think starting a thread and synchronously receiving is appropriate in this case.

I'd love to get the opinion of @aslushnikov and/or @ak239 - I believe they have suggested this approach.

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

I am not sure that I understand rust enough to figure out what is happening in this code.

Inspector assumes that all V8Inspector functions are called on the main thread.
In case if V8 is running some busy loop right now, there is a V8 API to request interrupt from any thread: https://cs.chromium.org/chromium/src/v8/include/v8.h?q=f:v8.h&sq=package:chromium&g=0&l=8222

I think that in Node on the new message we try to post a message to the main thread and RequestInterrupt at the same time, first who succeed will process a message.

This comment has been minimized.

Copy link
@eugeneo

eugeneo Jul 31, 2019

Relevant Node code: https://github.com/nodejs/node/blob/2abdfc5d7eba620a84a1aea3804beb6de28e19fa/src/inspector/main_thread_interface.cc#L227

We send RequestInterrupt in case Node is running continuous code block (e.g. while(true){}) and we trigger libuv in case Node.js is waiting for the IO event (e.g. web service waiting for a connection).

).arg(
Arg::with_name("inspect-brk")
.long("inspect-brk")
.help("Enable inspector and pause on first statement")

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

Two flags are required for two different problems:
--inspect starts the process and allow the inspector to connect any time later, e.g. some tool would like to attach to a running program from time to time and capture one of the heap profilers to build aggregated report later,
--inspect-brk is for the clients which need to set up something ahead of time, e.g. set breakpoints or start profiler. In this mode inspected process should wait until inspector client is connected and sends Runtime.runIfWaitingForDebugger before run. At the same time, I am not sure that pause at first statement is an important part of this behavior.

If I would need to choose only one flag that covers most use cases, I will add --inspect-brk flag that waits for connected client and Runtime.runIfWaitingForDebugger protocol method without breakpoint at first statement.
DevTools or other clients can set required breakpoint manually - it should be possible to fetch entry file name after client connected and before it sent Runtime.runIfWaitingForDebugger

cli/flags.rs Outdated Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
use warp::ws::{Message, WebSocket};
use warp::Filter;

static UUID: &str = "97690037-256e-4e27-add0-915ca5421e2f";

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

This UUID should be unguessable and randomly generated for each separate run. There is no well-defined security model for the inspector clients but unique UUID provides some guarantees that you can not just ping the inspector port and start debugging deno process.

In Node and Chrome there are two ways to get full web socket debugger URL:

  • parse stderr of the Chrome or Node process, both of them dumps something like Debugger is listening on ws://..., some tools use this approach.
  • fetch 127.0.0.1:port/json/list to get json list, there is a flag to disable this endpoint in Node.

This comment has been minimized.

Copy link
@eugeneo

eugeneo Jul 31, 2019

Note that it is not just the local clients that may access localhost WebSocket server, but any web server. Just imaging malicious web site that publishes some Deno tutorial (to ensure that Deno users open the page) and then use Deno debug interface to read local FS.

user_ws_tx = user_ws_tx.send(Message::text(msg)).wait().unwrap();
}
}
});

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

I am not sure that I understand rust enough to figure out what is happening in this code.

Inspector assumes that all V8Inspector functions are called on the main thread.
In case if V8 is running some busy loop right now, there is a V8 API to request interrupt from any thread: https://cs.chromium.org/chromium/src/v8/include/v8.h?q=f:v8.h&sq=package:chromium&g=0&l=8222

I think that in Node on the new message we try to post a message to the main thread and RequestInterrupt at the same time, first who succeed will process a message.

}

void InspectorClient::runMessageLoopOnPause(int context_group_id) {
// TODO(mtharrison): Needs protection for nested loop?

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

Inspector runs this nested message loop on pause, we do not support nested pauses, so it should be no nested loops, but it is always better to have some protection.

InspectorClient(Local<Context> context, deno::DenoIsolate* deno_);
~InspectorClient() override = default;

void runMessageLoopOnPause(int context_group_id) override;

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

For DevTools and tools to work properly some other methods are useful:

  • ensureDefaultContextInGroup - method should return default v8::Context,
  • currentTimeMS - method is used by different profilers.

This comment has been minimized.

Copy link
@ak239

ak239 Jul 31, 2019

.. and definitely runIfWaitingForDebugger - this method is called when inspector backend is processing Runtime.runIfWaitinForDebugger protocol command.

"id": UUID,
"title": format!("deno[{}]", std::process::id()),
"type": "deno",
"url": "file://",

This comment has been minimized.

Copy link
@eugeneo

eugeneo Jul 31, 2019

I am not familiar with Deno, but in Node some script files V8 reports have relative URLs. Some frontends use this URL to resolve those paths.

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Thanks for all the feedback so far. I'm away this weekend on a trip but I'll start working on the suggestions on Monday 😄

@ry

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

We're working on building via GN in #2707 ... unfortunately warp adds a bunch of dependencies which force multiple versions of crates - which we want to avoid - so it's not so straightforward. (This also happens in the cargo build, it's just less visible.)

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Ok @ry if it causes too much pain we could probably look at another ws crate. What we need is pretty simple for this use case.

@mtharrison mtharrison force-pushed the mtharrison:inspector branch from aff61fa to a559921 Aug 7, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@mtharrison I tried running your PR but I always get this error:

thread 'main' panicked at 'error binding to 127.0.0.1:19888: error creating server listener: Address already in use (os error 48)', /Users/biwanczuk/.cargo/registry/src/github.com-1ecc6299db9ec823/warp-0.1.18/src/server.rs:206:27

Even if I change port for debugger it always panics like that. Did you ever encounter it?

EDIT: It only happens when running with --inspect flag, running with --inspect-brk is ok

@mtharrison mtharrison force-pushed the mtharrison:inspector branch from a559921 to 87b2228 Aug 8, 2019

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@bartlomieju I've changed the flags a bit now, in the process of just having a single --debug=[address] flag. Can you try again with ./target/debug/deno --debug=127.0.0.1:9888 run script.js.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@mtharrison unfortunately I can't build the branch, I believe some things went wrong when merging master:

../../core/libdeno/modules_test.cc:403:76: error: missing field 'inspector_message_cb' initializer [-Werror,-Wmissing-field-initializers]
  Deno* d = deno_new(deno_config{0, snapshot, empty, recv_cb, dyn_import_cb});
@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@bartlomieju I'm not seeing this error. Are you using cargo build?

piscisaureus and others added some commits Aug 9, 2019

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

FYI you can now do --debug run script.js and it will default to 127.0.0.1:9888 or specify your own address with --debug=127.0.0.1:5555 run script.js .

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

@bartlomieju I'm not seeing this error. Are you using cargo build?

It works now, very nice!

FYI you can now do --debug run script.js and it will default to 127.0.0.1:9888 or specify your own address with --debug=127.0.0.1:5555 run script.js .

👍

Review coming tonight

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Review coming tonight

Excellent. Looking forward to it. I'm going to carve out some time this weekend to work on this.

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

I managed to get 'warp' to build with gn/ninja after all. See #2707.

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@piscisaureus cool, thanks! I'll try to incorporate that into my branch.

.require_equals(true)
.min_values(0)
.value_name("ADDRESS")
.help("Enable debugger and pause on first statement"),

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 11, 2019

Contributor

Please add a more explicit note explaining how to use ADDRESS

let (ready_tx, ready_rx) = channel::<()>();

let address = match address {
Some(address) => address.parse::<SocketAddrV4>().unwrap(),

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 11, 2019

Contributor

If malformed address is passed then the process will panic and die. How about chaning signature to pub fn new(enable: bool, address: Option<String>) -> Result<Self, Errbox> and handling those unwraps like so: address.parse::<SocketAddrV4>()??

use warp::Filter;

pub struct Inspector {
enable: bool,

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 11, 2019

Contributor

What's the purpose of this field?

self.server_spawn_handle = Some(spawn(
self.serve(),
&tokio_executor::DefaultExecutor::current(),
));

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 11, 2019

Contributor

Does this method call spawn a new Tokio runtime?

i.setup_inspector();
}

// TODO(mtharrison): This is all wrong but I'm not sure how to structure it?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 11, 2019

Contributor

I don't get this bit, @mtharrison could you explain what's going on here?

@@ -30,6 +32,23 @@ pub fn v8_version() -> &'static str {
c_str.to_str().unwrap()
}

// TODO(mtharrison): Move these somewhere else

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 11, 2019

Contributor

//core/isolate.rs?

ry added some commits Aug 12, 2019

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

The ASAN build is reporting plenty of leaks with this branch. I suspect this is due to some thread never exiting cleanly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.