fix for windows debug support #14048

snoglobe · 2024-09-19T21:56:09Z

What does this PR do?

this fixes #8851, which pertains to the bun debugger not working on windows.

for windows platform, we now use websockets to communicate to the debugger and inspector. on unix platforms, we keep the existing unix sockets implementation.

Documentation or TypeScript types (it's okay to leave the rest blank in this case)
Code changes

How did you verify your code works?

tested it by debugging programs under vscode in windows

robobun · 2024-09-19T21:56:25Z

❌ @snoglobe, your commit ba54a17 has 7 failures in #3683:

test/js/node/watch/fs.watch.test.ts - 3 failing on 🍎 14 x64

test/js/web/streams/streams.test.js - 1 failing on 🍎 14 aarch64

test/js/web/streams/streams.test.js - 1 failing on 🍎 14 x64

test/js/web/fetch/fetch-tcp-stress.test.ts - timeout on 🍎 13 aarch64

test/js/web/fetch/fetch-tcp-stress.test.ts - timeout on 🍎 14 x64

test/cli/install/registry/bun-install-registry.test.ts - 2 failing on 🍎 14 aarch64

test/js/node/test/parallel/child-process-exec-timeout-kill.test.js - 1 failing on 🍎 13 aarch64

test/cli/hot/watch.test.ts - 1 failing on 🪟 x64

test/cli/hot/watch.test.ts - 1 failing on 🪟 x64-baseline

test/cli/watch/watch.test.ts - 2 failing on 🪟 x64-baseline

test/cli/watch/watch.test.ts - 2 failing on 🪟 x64

snoglobe · 2024-09-19T23:00:10Z

note: failed builds are due to buildkite

Jarred-Sumner · 2024-09-20T00:03:46Z

packages/bun-debug-adapter-protocol/src/debugger/adapter.ts

 import { Location, SourceMap } from "./sourcemap";
+import { createServer, AddressInfo } from "node:net";
+
+export function getAvailablePort(): number {


Do we need to wait for the callback to be called?

it works fine without waiting for callback

does node gurantee that the address will always be set before the listen callback is invoked?

server.address() returns null before the 'listening' event has been emitted or after calling server.close().

Jarred-Sumner · 2024-09-20T00:06:52Z

packages/bun-vscode/scripts/build.mjs

@@ -9,7 +9,7 @@ buildSync({
  entryPoints: ["src/extension.ts", "src/web-extension.ts"],
  outdir: "dist",
  bundle: true,
-  external: ["vscode"],
+  external: ["vscode", "ws"],


Is this intentional?

snoglobe · 2024-09-20T00:08:28Z

yes; build fails otherwise

…

On Thu, Sep 19, 2024 at 5:07 PM Jarred Sumner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/bun-vscode/scripts/build.mjs <#14048 (comment)>: > @@ -9,7 +9,7 @@ buildSync({ entryPoints: ["src/extension.ts", "src/web-extension.ts"], outdir: "dist", bundle: true, - external: ["vscode"], + external: ["vscode", "ws"], Is this intentional? — Reply to this email directly, view it on GitHub <#14048 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGC3MRFXZGREDAC32PERZDLZXNRLFAVCNFSM6AAAAABOQZ3VNCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMJWHEZTQNRYGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Jarred-Sumner · 2024-09-20T00:10:02Z

What happens when you build the installed extension and the ws package isn’t installed?

…

On Thu, Sep 19, 2024 at 5:08 PM snwy ***@***.***> wrote: yes; build fails otherwise On Thu, Sep 19, 2024 at 5:07 PM Jarred Sumner ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In packages/bun-vscode/scripts/build.mjs > <#14048 (comment)>: > > > @@ -9,7 +9,7 @@ buildSync({ > entryPoints: ["src/extension.ts", "src/web-extension.ts"], > outdir: "dist", > bundle: true, > - external: ["vscode"], > + external: ["vscode", "ws"], > > Is this intentional? > > — > Reply to this email directly, view it on GitHub > <#14048 (review)>, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AGC3MRFXZGREDAC32PERZDLZXNRLFAVCNFSM6AAAAABOQZ3VNCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMJWHEZTQNRYGE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > — Reply to this email directly, view it on GitHub <#14048 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFNGS5BPUU3W6UTO4MHTELZXNRRFAVCNFSM6AAAAABOQZ3VNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRSGQ2DSMBYGA> . You are receiving this because you commented.Message ID: ***@***.***>

snoglobe · 2024-09-20T00:54:19Z

What happens when you build the installed extension and the ws package isn’t installed?

errors w/ the bundler unable to resolve ws

Jarred-Sumner · 2024-09-20T02:32:23Z

errors w/ the bundler unable to resolve ws

did you run bun install (or npm install)?

Jarred-Sumner · 2024-09-20T03:06:08Z

src/js/internal/debugger.ts

@@ -73,7 +83,7 @@ class Debugger {
  #listen(): void {
    const { protocol, hostname, port, pathname } = this.#url;

-    if (protocol === "ws:" || protocol === "ws+tcp:") {


If we're allowing wss, where is the TLS configuration specified?

Jarred-Sumner

Not mergeable until:

the listen() callback is addressed (it will be randomly flaky, in it's current version even if it appears to work sometimes when you run it locally)
The "ws" module should not be external. We bundle the VSCode extension instead of expecting a node_modules folder to be present (it won't be, except for development)

Separately, the "wss" validation is a little confusing unless the idea is you have a reverse proxy between Bun.serve() and the websocket connection? This is uncommon, as people usually don't proxy websockets but not impossible I suppose.

Jarred-Sumner · 2024-09-20T04:28:57Z

src/js/internal/debugger.ts

@@ -321,6 +331,18 @@ function reset(): string {
  return "";
 }

+function notifyWebSocket(url: string): void {


Oh, it's a whole websocket server for this? We can just make a TCP connection.

Jarred-Sumner · 2024-09-20T04:46:33Z

I want to get the Bun changes into the release, so going to merge that separately from this PR since the extension can be updated separately after

snoglobe · 2024-09-20T19:26:01Z

Not mergeable until:

the listen() callback is addressed (it will be randomly flaky, in it's current version even if it appears to work sometimes when you run it locally)

The "ws" module should not be external. We bundle the VSCode extension instead of expecting a node_modules folder to be present (it won't be, except for development)

Separately, the "wss" validation is a little confusing unless the idea is you have a reverse proxy between Bun.serve() and the websocket connection? This is uncommon, as people usually don't proxy websockets but not impossible I suppose.

will remove wss validation then; yeah not sure why i put it

snoglobe · 2024-09-20T19:26:31Z

errors w/ the bundler unable to resolve ws

did you run bun install (or npm install)?

yes

- removed wss

Jarred-Sumner · 2024-09-21T06:25:06Z

packages/bun-debug-adapter-protocol/src/debugger/adapter.ts

@@ -2149,10 +2207,10 @@ function titleize(name: string): string {
 }

 function sourcePresentationHint(url?: string): DAP.Source["presentationHint"] {
-  if (!url || !url.startsWith("/")) {
+  if (!url || path.isAbsolute(url)) {


!path.isAbsolute

packages/bun-debug-adapter-protocol/src/debugger/adapter.ts

packages/bun-vscode/src/features/debug.ts

packages/bun-debug-adapter-protocol/src/debugger/signal.ts

Jarred-Sumner · 2024-09-21T07:13:15Z

packages/bun-debug-adapter-protocol/src/debugger/adapter.ts

+
+      const query = stopOnEntry ? "break=1" : "wait=1";
+      processEnv["BUN_INSPECT"] = `${url}?${query}`;
+      processEnv["BUN_INSPECT_NOTIFY"] = `tcp://127.0.0.1:${signal.port}`; // 127.0.0.1 so it resolves correctly on windows


Suggested change

processEnv["BUN_INSPECT_NOTIFY"] = `tcp://127.0.0.1:${signal.port}`; // 127.0.0.1 so it resolves correctly on windows

processEnv["BUN_INSPECT_NOTIFY"] = signal.url

liudonghua123 · 2024-09-22T02:40:42Z

I see this pr changed unix domain socket to websocket through network ipc. It just like bun inspect cli debugging doing.

I tested some simple demos of unix domain socket on windows and it works. How about to fix existing unix domain socket for windows, it's more high performance in my opinion. It's just my own thought. 😄

Jarred-Sumner · 2024-09-22T02:42:00Z

@liudonghua123 you're welcome to try getting unix domain sockets working when used in VSCode on Windows

snoglobe added 3 commits September 17, 2024 19:48

debugger using websockets

49090cf

unix sockets on unix; websockets on windows

773a3ee

unix sockets on unix; websockets on windows

bc66697

Jarred-Sumner reviewed Sep 20, 2024

View reviewed changes

Jarred-Sumner requested changes Sep 20, 2024

View reviewed changes

Jarred-Sumner assigned snoglobe Sep 20, 2024

Jarred-Sumner reviewed Sep 20, 2024

View reviewed changes

Don't use a WebSocket for this

aea25a4

Jarred-Sumner force-pushed the debugger_ws branch from 6920246 to aea25a4 Compare September 20, 2024 04:42

Update debugger.ts

e291637

snoglobe and others added 7 commits September 20, 2024 14:38

- fixed getAvailablePort

5e6b000

- removed wss

ws not external

3d45d68

TRUE windows debug support

802b408

Merge remote-tracking branch 'origin/main' into debugger_ws

13518c4

fix debugger.ts

5d0275e

comments for 127.0.0.1

6a0ad7f

remove extraneous return

32c5479