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

Initial support for devtools inspector protocol #17

Merged
merged 2 commits into from Oct 5, 2022
Merged

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 24, 2022

A work-in-progress prototype enabling devtools inspector protocol support for workerd.

Adds an additional option to workerd serve to enable a local inspector port:

$ ./workerd serve -i 127.0.0.1:9229 config.capnp

(The inspector socket can also be passed as an fd instead)

When enabled, the inspector protocol wil be available on the given address and will be discoverable in devtools.

image

Each worker in a configuration will be listed separately in the devtools discovery view. The name of the worker itself will be displayed in the discovered list to make it possible to distinguish.

Very rudimentary for now.

This PR also adds support for using devtools to capture heap snapshots.

@jasnell
Copy link
Member Author

jasnell commented Sep 24, 2022

One issue this currently has is an error stack that is printed to the terminal when an attached debugger disconnects:

workerd/server/server.c++:1638: error: Uncaught exception: kj/compat/http.c++:2398: disconnected: WebSocket disconnected between frames without sending `Close`.
stack: /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@2aac760 /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6fd8e10 /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6fdd2f0
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
workerd/server/server.c++:1564: error: exception = kj/compat/http.c++:1969: failed: expected !inBody; previous HTTP message body incomplete; can't write more messages
stack: /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6f776cc /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@70009d9 /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6f3946f /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6f3963d /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@241d998 /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@241d9e3 /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6fdd52b /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6fdd44c /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6fddb6f /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@6fdd8b0 /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@72a9350 /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@72a9328 /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@737dcef /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@72a1a4c /home/james/.cache/bazel/_bazel_james/728d89b94395895921db6cbedc832917/execroot/workerd/bazel-out/k8-fastbuild/bin/src/workerd/server/workerd@2c29060
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here
    ??:0: returning here

@jasnell
Copy link
Member Author

jasnell commented Sep 24, 2022

Another issue is that memory profiling does not work. The devtools client will say that it's recording the snapshot but it never completes.

@jasnell jasnell mentioned this pull request Sep 24, 2022
@kentonv
Copy link
Member

kentonv commented Sep 25, 2022

I would have opened this as a draft PR but it seems we don't have that feature available.

FWIW apparently this is some sort of premium feature but will become available when the repo is public.

@jasnell
Copy link
Member Author

jasnell commented Sep 26, 2022

Ok, so I updated the implementation so that it is not added to the list of named/registered services but it still implements from Service. The reason is that it's just easier to implement this way since all of the mechanism for integrating Service and Socket are already in place.

I've also made it so that disconnects are handled cleanly and added some logging when --verbose is used. Fixed a couple of other bugs also. This should be ready to go for the initial rudimentary version.

@jasnell jasnell changed the title [WIP] Support for devtools inspector protocol Initial support for devtools inspector protocol Sep 26, 2022
@jasnell
Copy link
Member Author

jasnell commented Sep 26, 2022

Doh, conflict with one of the other changes... pushing a fix in a sec

Updated

@kentonv
Copy link
Member

kentonv commented Sep 26, 2022

Ok, so I updated the implementation so that it is not added to the list of named/registered services but it still implements from Service. The reason is that it's just easier to implement this way since all of the mechanism for integrating Service and Socket are already in place.

But you aren't actually using config::Socket to configure these sockets so there's no reason to use the socket code in the first place, and therefore no need to define the inspector in a way that integrates with that.

You should be able to use kj::HttpServer::listenHttp() directly, you don't need to use Server::listenHttp() which exists mainly to support those config options you're not using....

@Electroid Electroid linked an issue Sep 26, 2022 that may be closed by this pull request
@Electroid Electroid added the feature Implementation of a new feature label Sep 26, 2022
@jasnell jasnell force-pushed the jsnell/inspector branch 2 times, most recently from 0af087c to 10b7d23 Compare September 26, 2022 23:34
@jasnell
Copy link
Member Author

jasnell commented Sep 26, 2022

But you aren't actually using config::Socket to configure these sockets so there's no reason to use the socket code in the first place...

Yes, I know, but the mechanism was there and very simple to extend from. Easiest thing that just worked.

I've reworked it as you've suggested.

src/workerd/server/server.h Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/workerd.c++ Outdated Show resolved Hide resolved
src/workerd/io/worker.c++ Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Oct 1, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@jasnell
Copy link
Member Author

jasnell commented Oct 1, 2022

Bad CLA-bot! Bad! No I will not sign the CLA. (for those reading...I don't have to since I'm Cloudflare already...)

@kentonv
Copy link
Member

kentonv commented Oct 3, 2022

@jasnell Its gonna be easier if you just write the comment it wants you to write. You could also edit yourself into the allow list but that seems like more work.

src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Oct 4, 2022

I have read the CLA Document and I hereby sign the CLA

@jasnell
Copy link
Member Author

jasnell commented Oct 4, 2022

(silly CLA bot... you annoy me already)

github-actions bot added a commit that referenced this pull request Oct 4, 2022
Initial support for devtools inspector protocol for workerd.

Adds an additional option to `workerd serve` to enable a local inspector
port:

```bash
$ ./workerd serve -i 127.0.0.1:9229 config.capnp
```

When enabled, the inspector protocol wil be available on the given
address and will be discoverable in devtools.

Very rudimentary for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Implementation of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to support inspector?
4 participants