Support for breakpoint debugging #710

ohodson · 2023-05-26T08:12:20Z

No description provided.

src/workerd/io/worker.c++

ohodson · 2023-06-30T13:47:55Z

PTAL, this is a moderately large change that we'll likely have to iterate on quite a bit.

The major shortcoming here is that this is only worker for single service configs. This should be fixable, but it would be good to get feedback in the interim. The specific example that fails is ssh://git@bitbucket.cfdata.org:7999/~bcoll/workerd-bus-error-10.git

Thanks.

ohodson · 2023-07-03T14:30:09Z

The major shortcoming here is that this is only worker for single service configs. This should be fixable, but it would be good to get feedback in the interim. The specific example that fails is ssh://git@bitbucket.cfdata.org:7999/~bcoll/workerd-bus-error-10.git

This is addressed in the second commit. Wrangler does not expect to package multiple user written workers in a config, but may in practice (e.g. code that wrangler itself adds). With the second commit chrome devtools would be able to debug any code referenced by the config. Wrangler devtools only wants to be able to debug the user written workers (except if they need to debug their stuff).

src/workerd/io/worker.c++

kentonv

Just started looking at this, realize I need more time than I have now, so posting the couple comments I already wrote but I'll try to look at the whole thing either later today or tomorrow.

src/workerd/server/server.c++

src/workerd/io/worker.c++

kentonv

(still reading, but posting a batch before going to more meetings...)

src/workerd/io/worker.c++

kentonv

I had some review comments that seem to have disappeared, but I also notice that github says I have a pending review, but it says there are no comments, but maybe if I post it anyway they will come back? Trying...

kentonv · 2023-07-14T17:43:14Z

Sadly, it appears my comments were lost. Ugh.

src/workerd/io/worker.c++

ohodson · 2023-07-21T07:57:06Z

@kentonv, friendly ping, last comment incorporated on 7/14, thanks.

kentonv

I assume you've tested this running in the fiddle service (internal codebase)? That should be a pretty easy way to verify it works with the internal threading model, in addition to the workerd model.

src/workerd/io/worker.c++

kentonv · 2023-07-24T20:27:52Z

(Sorry for the slow review, this fell off my radar last week somehow.)

paplco · 2023-07-28T08:49:05Z

awesome, approved!
hopefully this fixes setting breakpoints in vscode
is there a target release to include this PR?
Thanks!

ohodson · 2023-07-31T12:06:38Z

I assume you've tested this running in the fiddle service (internal codebase)? That should be a pretty easy way to verify it works with the internal threading model, in addition to the workerd model.

Yes, you can set breakpoints and single step. Logging to console and heap querying appear to still work as before.

kentonv · 2023-07-31T20:03:59Z

src/workerd/io/worker.c++

+        try {
+          auto lockedOutgoingQueue = outgoingQueue.lockExclusive();
+          if (lockedOutgoingQueue->status == MessageQueue::Status::CLOSED) {
+            co_await webSocket.close(1000, "client closed connection");


It looks like the behavior changed slightly in the coroutine conversion, previously we would send any remaining messages before closing.

Ack, fixed, thanks.

kentonv · 2023-07-31T20:05:35Z

src/workerd/io/worker.c++

+      for (;;) {
+        co_await outgoingQueueNotifier->awaitNotification();
+        try {
+          auto lockedOutgoingQueue = outgoingQueue.lockExclusive();


You need to drop this lock before co_awaiting.

This change places the InspectorService in a separate thread that manages communication with over the websocket to the inspector. The change also adds support for runMessageLoopOnPause() and quitMessageLoopOnPause() to support breakpoints and debugger break statements. There is also refactoring of the CDP message handling code so it can be called with or without the isolate lock held. This requires workerd to run with the command-line flag -i to turn on inspector support. This change only works for single service configurations. Support for multi service configurations to follow. To try this out using samples/helloworld as an example: 1) Edit "samples/helloworld/worker.js" and add a debugger statement to the handle(request) method. 2) Open workerd in VSCode, select 'workerd with inspector enabled (dbg)' as the Run and Debug Target panel. Hit F5 to run and select `samples/helloworld/config.capnp` as the config to use. 3) Open devtools in Chrome using either: * https://devtools.devprod.cloudflare.dev/js_app?ws=localhost:9229/main * chrome:://inspect 4) On the command-line run, `curl http://localhost:8080/` 5) Devtools should break into the running worker. Bug: #371 Test: manual Test: existing internal ew tests do not break

If there are multiple service workers in a config, expose them to devtools. This is largely just for Chrome devtools since wrangler does not expect to generate more than one worker per config that might need to be debugged. Test: manual using chrome devtools and https://bitbucket.cfdata.org/users/bcoll/repos/workerd-bus-error-10/browse

With cloudflare/workerd#710, `workerd` supports breakpoint debugging! Support for this in Miniflare just worked, assuming you were using a plain JavaScript worker, or you had inline source maps. `workerd` doesn't know where workers are located on disk, it just knows files' locations relative to each other. This means it's unable to resolve locations of corresponding linked `.map` files in `sourceMappingURL` comments. Miniflare _does_ have this information though. This change detects linked source maps and rewrites `sourceMappingURL` comments to `http` URLs pointing to Miniflare's loopback server. This then looks for the source map relative to the known on-disk source location. Source maps' `sourceRoot` attributes are updated to ensure correct locations are displayed in DevTools. **This enables breakpoint debugging for compiled TypeScript with linked source maps!** 🎉 Closes DEVX-872

V8's profiler does not expect isolates to be locked from different threads. Pull request #710 moved inspector protocol handling to a separate thread, meaning profiling was started and stopped on the inspector thread, not the Worker's request connection thread. If I remember right, the result of this is that the profiler ends up sampling the inspector thread, which doesn't normally run JavaScript, and causes a bunch of "(program)" placeholder spans to show up in the profile. We previously discovered this issue with our internal runtime, which has always been heavily multi-threaded, and patched around the problem. This commit copies the two relevant patches from our internal repo. Fixes #1754.

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch 2 times, most recently from d4e73da to 6586c4d Compare June 5, 2023 16:20

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch 2 times, most recently from 50fc6e4 to b776b91 Compare June 20, 2023 09:49

ohodson commented Jun 20, 2023

View reviewed changes

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch 5 times, most recently from af925c7 to f5afe97 Compare June 30, 2023 13:40

ohodson changed the title ~~Not For Commit: breakpoint debugging prototype~~ Initial support for breakpoint debugging Jun 30, 2023

ohodson marked this pull request as ready for review June 30, 2023 13:43

ohodson requested review from kentonv and jasnell June 30, 2023 13:44

ohodson changed the title ~~Initial support for breakpoint debugging~~ Support for breakpoint debugging Jul 3, 2023

jasnell reviewed Jul 3, 2023

View reviewed changes

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

jasnell reviewed Jul 3, 2023

View reviewed changes

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

jasnell reviewed Jul 3, 2023

View reviewed changes

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

jasnell approved these changes Jul 3, 2023

View reviewed changes

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch 2 times, most recently from 96940d3 to dea77d4 Compare July 5, 2023 14:40

kentonv reviewed Jul 5, 2023

View reviewed changes

src/workerd/server/server.c++ Outdated Show resolved Hide resolved

src/workerd/server/server.c++ Show resolved Hide resolved

src/workerd/server/server.c++ Outdated Show resolved Hide resolved

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

kentonv reviewed Jul 5, 2023

View reviewed changes

src/workerd/io/worker.c++ Show resolved Hide resolved

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch 4 times, most recently from 512bd36 to 1063006 Compare July 6, 2023 14:47

kentonv reviewed Jul 14, 2023

View reviewed changes

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch 2 times, most recently from a4d477b to 6409cca Compare July 14, 2023 19:59

huw mentioned this pull request Jul 19, 2023

🚀 Feature Request: Breakpoint debugging support in workerd #371

Closed

kentonv approved these changes Jul 24, 2023

View reviewed changes

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

src/workerd/io/worker.c++ Show resolved Hide resolved

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch from 6409cca to f937d74 Compare July 31, 2023 10:38

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch from f937d74 to f9aa9e6 Compare July 31, 2023 14:09

kentonv reviewed Jul 31, 2023

View reviewed changes

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch from f9aa9e6 to fb85369 Compare August 1, 2023 08:52

ohodson added 2 commits August 1, 2023 10:23

ohodson force-pushed the orion/breakpoint-debugging-pt2 branch from fb85369 to 45a8779 Compare August 1, 2023 09:23

ohodson merged commit eba4bea into main Aug 1, 2023
7 checks passed

ohodson deleted the orion/breakpoint-debugging-pt2 branch August 1, 2023 09:58

mrbbot mentioned this pull request Aug 15, 2023

[Miniflare 3] Serve linked source maps from loopback server (enable breakpoint debugging for TypeScript) cloudflare/miniflare#660

Merged

harrishancock mentioned this pull request Jun 6, 2024

Add V8 cross-thread profiling patches #2227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for breakpoint debugging #710

Support for breakpoint debugging #710

ohodson commented May 26, 2023

ohodson commented Jun 30, 2023 •

edited

Loading

ohodson commented Jul 3, 2023 •

edited

Loading

kentonv left a comment

kentonv left a comment

kentonv left a comment

kentonv commented Jul 14, 2023

ohodson commented Jul 21, 2023

kentonv left a comment

kentonv commented Jul 24, 2023

paplco commented Jul 28, 2023

ohodson commented Jul 31, 2023

kentonv Jul 31, 2023

ohodson Aug 1, 2023

kentonv Jul 31, 2023

ohodson Aug 1, 2023

Support for breakpoint debugging #710

Support for breakpoint debugging #710

Conversation

ohodson commented May 26, 2023

ohodson commented Jun 30, 2023 • edited Loading

ohodson commented Jul 3, 2023 • edited Loading

kentonv left a comment

Choose a reason for hiding this comment

kentonv left a comment

Choose a reason for hiding this comment

kentonv left a comment

Choose a reason for hiding this comment

kentonv commented Jul 14, 2023

ohodson commented Jul 21, 2023

kentonv left a comment

Choose a reason for hiding this comment

kentonv commented Jul 24, 2023

paplco commented Jul 28, 2023

ohodson commented Jul 31, 2023

kentonv Jul 31, 2023

Choose a reason for hiding this comment

ohodson Aug 1, 2023

Choose a reason for hiding this comment

kentonv Jul 31, 2023

Choose a reason for hiding this comment

ohodson Aug 1, 2023

Choose a reason for hiding this comment

ohodson commented Jun 30, 2023 •

edited

Loading

ohodson commented Jul 3, 2023 •

edited

Loading