startDevWorker - Milestone 1 #3960

RamIdeas · 2023-09-15T18:48:19Z

What this PR solves / how to test:

This PR implements the first milestone for startDevWorker. This includes the ProxyController, ProxyWorker and ProxyInspectorWorker. The startDevWorker function is not exposed yet – the components implemented so far have been inserted tactfully 👀 into the existing flows for wrangler dev, wrangler dev --remote, unstable_dev(...).

There should be no breaking changes. Improvements include:

no more port in use errors upon reloads (except for an existing race condition we'll fix in a quick follow-up)
upon config/source file changes, requests are buffered to guarantee the response is from the new version of the worker

Author has included the following, where applicable:

Tests
Changeset (Changeset guidelines)

Reviewer is to perform the following, as applicable:

Checked for inclusion of relevant tests
Checked for inclusion of a relevant changeset
Checked for creation of associated docs updates
Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

changeset-bot · 2023-09-15T18:48:22Z

🦋 Changeset detected

Latest commit: 334bfa0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/wrangler/package.json

packages/wrangler/scripts/bundle.ts

packages/wrangler/src/__tests__/jest.setup.ts

RamIdeas · 2023-09-15T19:17:28Z

packages/wrangler/package.json

@@ -60,7 +60,7 @@
 		"emit-types": "tsc -p tsconfig.emit.json && node -r esbuild-register scripts/emit-types.ts",
 		"prepublishOnly": "SOURCEMAPS=false npm run build",
 		"start": "pnpm run bundle && cross-env NODE_OPTIONS=--enable-source-maps ./bin/wrangler.js",
-		"test": "pnpm run assert-git-version && jest",
+		"test": "pnpm run assert-git-version && jest --runInBand",


Had to do this to avoid a race condition where every test thought it could start the InspectorProxyWorker on 9229 because get-port doesn't work across processes (jest workers, in this case)

We confirmed this affects wrangler dev and unstable_dev if you run multiple processes in parallel where get-port thinks the requested port is available but then there's a race for all the workerd instances to bind to it

We have a fix coming in a follow-up

TODO: make miniflare surface "port in use" errors so wrangler can retry instantiating Miniflare with port 0

RamIdeas · 2023-09-15T19:20:48Z

packages/wrangler/src/api/dev.ts

@@ -69,6 +70,7 @@ export interface UnstableDevOptions {
 export interface UnstableDevWorker {
 	port: number;
 	address: string;
+	proxyData: ProxyData;


Adding to this public interface so we can fake some startDevWorker events (call the event listeners)

TODO add getBindings to this interface

RamIdeas · 2023-09-15T19:23:28Z

packages/wrangler/src/api/startDevWorker/DevEnv.ts

+/**
+ * @internal
+ */
+export class DevEnv extends EventEmitter {


This class contains the glue code for all startDevWorker controllers. This is the contract between all controllers. You could override the instances of each/any controller in the constructor but we're not exposing this to users yet until we're more sure of the contract.

packages/wrangler/src/api/startDevWorker/DevEnv.ts

JacobMGEvans

First pass, tentatively, looks GREAT! I will probably go over it again maybe even jump on a call with @RamIdeas for a pair review for a 2nd pass!

This PR also should have a couple approvals minimum.

RamIdeas · 2023-09-15T19:29:08Z

packages/wrangler/src/api/startDevWorker/devtools.ts

@@ -0,0 +1,41 @@
+import type Protocol from "devtools-protocol/types/protocol-mapping";


The types from devtools-protocol don't expose the full object we see in the websocket messages, just the sub-properties for some reason

This file may seem daunting but it just coerces the types into the whole object we see – e.g. with method and params matched together not as separate type unions which is basically useless

@mrbbot got nerd-sniped and got it working 👏

RamIdeas · 2023-09-15T19:34:16Z

packages/wrangler/src/dev/dev.tsx

@@ -244,7 +255,55 @@ type DevSessionProps = DevProps & {
 };

 function DevSession(props: DevSessionProps) {


This is (essentially) the entry-point to wrangler dev – here we create a DevEnv and map the props into StartDevWorkerOptions (at least what is relevant for now – for ProxyController) and fake events by calling ProxyController event handler methods (devEnv.proxy.on*)

RamIdeas · 2023-09-15T19:36:51Z

packages/wrangler/src/dev/dev.tsx

+			initialPort={0} // hard-code for userworker, DevEnv-ProxyWorker now uses this prop value
+			initialIp={"127.0.0.1"} // hard-code for userworker, DevEnv-ProxyWorker now uses this prop value
 			rules={props.rules}
 			inspectorPort={props.inspectorPort}
 			runtimeInspectorPort={props.runtimeInspectorPort}
 			localPersistencePath={props.localPersistencePath}
 			liveReload={props.liveReload}
 			crons={props.crons}
 			queueConsumers={props.queueConsumers}
-			localProtocol={props.localProtocol}
+			localProtocol={"http"} // hard-code for userworker, DevEnv-ProxyWorker now uses this prop value


The user worker now never starts on https, and binds to 127.0.0.1 on a random port. The ProxyWorker will start on the previous address values

RamIdeas · 2023-09-15T19:41:34Z

packages/wrangler/src/dev/remote.tsx

-	usePreviewServer({
-		previewToken,
-		assetDirectory: props.isWorkersSite
-			? undefined
-			: props.assetPaths?.assetDirectory,
-		localProtocol: props.localProtocol,
-		localPort: props.port,
-		ip: props.ip,
-	});
-
-	useInspector({
-		inspectorUrl:
-			props.inspect && previewToken
-				? previewToken.inspectorUrl.href
-				: undefined,
-		port: props.inspectorPort,
-		logToTerminal: true,
-		sourceMapPath: props.sourceMapPath,
-		host: previewToken?.host,
-		name: props.name,
-		sourceMapMetadata: props.bundle?.sourceMapMetadata,
-	});
-


No more preview server or inspector server, replaced by ProxyWorker and ProxyInspectorWorker respectively

RamIdeas · 2023-09-15T19:43:36Z

packages/wrangler/src/dev/start-server.ts

@@ -72,6 +74,34 @@ export async function startDevServer(
 		}
 	}

+	const devEnv = new DevEnv();


This outer function (startDevServer) is called internally by unstable_dev. Here we create a DevEnv and fake events to the ProxyController by calling it's event handler methods (devEnv.proxy.on*)

RamIdeas · 2023-09-15T19:45:34Z

packages/wrangler/src/https-options.ts

 	// `selfsigned` imports `node-forge`, which is a pretty big library.
 	// To reduce startup time, only load this dynamically when needed.
 	// eslint-disable-next-line @typescript-eslint/consistent-type-imports
-	const generate: typeof import("selfsigned").generate = promisify(


This is one of those fake async libs where they provided a node.js callback signature for ecosystem consistency. It makes no difference to call it without a callback ("synchronously") but this way we can de-async the callstack

Alternatively, could we take advantage of the existing async nature of this function to use an ESM await import()? Reducing synchronous require()s will be helpful with things like e.g. a future move to Viteste

I think await import() segfaults in Jest, breaking our existing tests 🙁

packages/wrangler/src/inspect.ts

RamIdeas · 2023-09-15T19:48:02Z

packages/wrangler/src/worker.d.ts

@@ -0,0 +1,4 @@
+declare module "worker:*" {


Declaring the type for worker: prefix module imports. We added an esbuild plugin to find these worker files in packages/wrangler/templates/*

RamIdeas · 2023-09-15T19:49:20Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

+		this.requestQueue.set(request, promise);
+		this.processQueue();
+
+		return promise;


Synchronously returning this promise. The promise will resolve later when we process the queue.

Any possible things running that would depend on this Promise? Considering possible race conditions or the like.

RamIdeas · 2023-09-15T19:50:27Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

+	processProxyControllerRequest(request: Request) {
+		const event = request.cf?.hostMetadata;
+		switch (event?.type) {
+			case "pause":


pause events cause the ProxyWorker to buffer requests

RamIdeas · 2023-09-15T19:51:14Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

+				this.proxyData = undefined;
+				break;
+
+			case "play":


play events cause the ProxyWorker to begin proxying requests to the UserWorker (using proxyData)

RamIdeas · 2023-09-15T19:52:07Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

+		await env.PROXY_CONTROLLER.fetch("http://dummy", {
+			method: "POST",
+			body: JSON.stringify(message),
+		});


The ProxyWorker can communicate back to the ProxyController via this service binding

RamIdeas · 2023-09-15T20:00:44Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

+				}
+			}
+
+			void fetch(url, new Request(req, { headers }))


Not awaiting this fetch means we replay the requests in order but we don't guarantee their delivery in order. We could await just the headers (not the body) to guarantee delivery order too but I don't think it matters too much – happy to hear other opinions

If you are chaining then & catch, why is the void necessary?

It shouldn't be a floating promise or anything, if its been chained with a catch I mean.

Just being explicit about not awaiting the promise since we're in a loop and it's on purpose

But I guess I could just add a comment instead

Is there any reason not to await the fetch? We're already creating an ordered buffer of requests, we might as well make sure that the delivery order is preserved

The main reason is that if the UserWorker has any long-running requests and therefore requires parallel request handling, await-ing would prevent that.

I've left a comment stating, if we choose to await, we should also race it against a timeout of e.g. 100ms

RamIdeas · 2023-09-15T20:01:29Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

+						res = insertLiveReloadScript(req, res, this.env, proxyData);
+					}
+
+					deferred.resolve(res);


This resolves the promise returned by the initial fetch handler

RamIdeas · 2023-09-15T20:01:45Z

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

+						},
+					});
+
+					deferred.reject(error);


This rejects the promise returned by the initial fetch handler

packages/wrangler/templates/startDevWorker/InspectorProxyWorker.ts

RamIdeas · 2023-09-15T20:06:25Z

packages/wrangler/templates/startDevWorker/InspectorProxyWorker.ts

+	},
+} as ExportedHandler<Env>;
+
+export class InspectorProxyWorker implements DurableObject {


This is a big boi, sorry

It's job is to proxy messages between the runtime inspector (#websockets.runtime), ProxyController (#websockets.proxyController) and devtools client (#websockets.devtools).

Because only one client can be connected to the runtime inspector server, the InspectorProxyWorker is essentially that one client. The ProxyController and devtools then can both connect to the InspectorProxyWorker.

but swallow errors if thrown

for ProxyController -> (Inspector)ProxyWorker requests to aid debugging

packages/wrangler/src/api/startDevWorker/DevEnv.ts

packages/wrangler/src/api/startDevWorker/ProxyController.ts

packages/wrangler/src/worker.d.ts

packages/wrangler/templates/startDevWorker/InspectorProxyWorker.ts

without requiring ProxyWorker restart

1000hz · 2023-10-12T15:22:38Z

packages/wrangler/src/api/startDevWorker/utils.ts

+
+	// if passed a previousDeferred, ensure it is resolved with the newDeferred
+	// so that await-ers of previousDeferred are now await-ing newDeferred
+	previousDeferred?.resolve(newPromise);


I'm a bit unclear on the use-case for chaining deferreds. If a deferred gets inherited, any holder of the previous deferred loses control over its lifecycle (the resolvers become inert since its promise has now already been resolved). Similarly, if the previous deferred has already been resolved before it gets inherited, the new deferred isn't allowed to extend the lifecycle of the previous one.

So whether or not a holder of a deferred maintains control over its lifecycle, and whether or not its lifecycle has been extended is unreliable. This seems error-prone and like it would lead to some insidious bugs.

If a deferred gets inherited, any holder of the previous deferred loses control over its lifecycle (the resolvers become inert since its promise has now already been resolved)

True

Similarly, if the previous deferred has already been resolved before it gets inherited, the new deferred isn't allowed to extend the lifecycle of the previous one.

True

So whether or not a holder of a deferred maintains control over its lifecycle, and whether or not its lifecycle has been extended is unreliable. This seems error-prone and like it would lead to some insidious bugs.

The point is not to extend the lifecycle of the old promise. It's to ensure the old promise is not left uncompleted. If the old promise is already completed (resolved/rejected), this line has no effect

This reverts commit c36b78b.

RamIdeas requested a review from a team as a code owner September 15, 2023 18:48

RamIdeas force-pushed the startDevWorker-milestone-1 branch from a179b91 to 51f8854 Compare September 15, 2023 18:53

JacobMGEvans reviewed Sep 15, 2023

View reviewed changes

packages/wrangler/package.json Outdated Show resolved Hide resolved

JacobMGEvans reviewed Sep 15, 2023

View reviewed changes

packages/wrangler/scripts/bundle.ts Show resolved Hide resolved

JacobMGEvans reviewed Sep 15, 2023

View reviewed changes

packages/wrangler/scripts/bundle.ts Show resolved Hide resolved

JacobMGEvans reviewed Sep 15, 2023

View reviewed changes

packages/wrangler/src/__tests__/jest.setup.ts Show resolved Hide resolved

RamIdeas commented Sep 15, 2023

View reviewed changes

packages/wrangler/src/api/startDevWorker/DevEnv.ts Outdated Show resolved Hide resolved

JacobMGEvans approved these changes Sep 15, 2023

View reviewed changes

RamIdeas commented Sep 15, 2023

View reviewed changes

packages/wrangler/src/inspect.ts Outdated Show resolved Hide resolved

RamIdeas commented Sep 15, 2023

View reviewed changes

packages/wrangler/src/inspect.ts Outdated Show resolved Hide resolved

RamIdeas commented Sep 15, 2023

View reviewed changes

packages/wrangler/templates/startDevWorker/InspectorProxyWorker.ts Outdated Show resolved Hide resolved

RamIdeas commented Sep 15, 2023

View reviewed changes

RamIdeas added 4 commits October 11, 2023 16:53

fix checks

ef0645c

on teardown, explicitly close websocket

ea3896a

but swallow errors if thrown

check torndown when handling sendMessageToProxyWorker errors

c0e9837

handle DevEnv error events with logger.error

8863445

RamIdeas force-pushed the startDevWorker-milestone-1 branch from 247cf2c to 8863445 Compare October 11, 2023 15:54

RamIdeas added 5 commits October 11, 2023 17:02

fix linting

aa138db

fix #cf-error-details typo

8edc039

fix WebSocket protocol parameter type

979b08c

fix InspectorProxyWorker comments

5229f15

use more descriptive request urls

be99b28

for ProxyController -> (Inspector)ProxyWorker requests to aid debugging

mrbbot approved these changes Oct 12, 2023

View reviewed changes

penalosa approved these changes Oct 12, 2023

View reviewed changes

RamIdeas added 5 commits October 12, 2023 14:24

remove DevEnv#workers

4faa1b1

fix request.url inside the UserWorker

0ed00d1

without requiring ProxyWorker restart

fix ProxyController error handling

cb65ab0

add changeset

8c8e2cb

add comment for why file: -> wrangler-file:

334bfa0

1000hz reviewed Oct 12, 2023

View reviewed changes

RamIdeas merged commit c36b78b into main Oct 12, 2023

RamIdeas deleted the startDevWorker-milestone-1 branch October 12, 2023 15:26

workers-devprod mentioned this pull request Oct 12, 2023

Version Packages #4160

Merged

penalosa added a commit that referenced this pull request Oct 12, 2023

Revert "startDevWorker - Milestone 1 (#3960)"

11a668a

This reverts commit c36b78b.

penalosa mentioned this pull request Oct 12, 2023

Revert "startDevWorker - Milestone 1" #4171

Merged

workers-devprod mentioned this pull request Oct 12, 2023

Version Packages #4163

Merged

RamIdeas mentioned this pull request Oct 13, 2023

startDevWorker Milestone 1 - Reboot #4175

Closed

8 tasks

RamIdeas mentioned this pull request Nov 13, 2023

startDevWorker Milestone 1 - Reboot #4413

Merged

6 tasks

lrapoport-cf mentioned this pull request Nov 21, 2023

Remove React and Ink from wrangler dev #2714

Closed

admah mentioned this pull request Jan 9, 2024

🚀 Stabilizing unstable_dev and expanding Wrangler as an API #4730

Closed

admah added this to the Wrangler as a Library milestone Jan 9, 2024

lrapoport-cf linked an issue Jan 10, 2024 that may be closed by this pull request

🚀 Stabilizing unstable_dev and expanding Wrangler as an API #4730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

startDevWorker - Milestone 1 #3960

startDevWorker - Milestone 1 #3960

RamIdeas commented Sep 15, 2023 •

edited

Loading

changeset-bot bot commented Sep 15, 2023 •

edited

Loading

RamIdeas Sep 15, 2023 •

edited

Loading

RamIdeas Oct 4, 2023

RamIdeas Sep 15, 2023

RamIdeas Oct 4, 2023

RamIdeas Sep 15, 2023

JacobMGEvans left a comment •

edited

Loading

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023 •

edited

Loading

penalosa Oct 6, 2023

mrbbot Oct 6, 2023 •

edited

Loading

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023

JacobMGEvans Sep 18, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023 •

edited

Loading

JacobMGEvans Sep 18, 2023

JacobMGEvans Sep 18, 2023

RamIdeas Sep 18, 2023

penalosa Oct 6, 2023

RamIdeas Oct 11, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023

RamIdeas Sep 15, 2023 •

edited

Loading

1000hz Oct 12, 2023

RamIdeas Oct 12, 2023

		@@ -0,0 +1,41 @@
		import type Protocol from "devtools-protocol/types/protocol-mapping";

		@@ -244,7 +255,55 @@ type DevSessionProps = DevProps & {
		};

		function DevSession(props: DevSessionProps) {

startDevWorker - Milestone 1 #3960

startDevWorker - Milestone 1 #3960

Conversation

RamIdeas commented Sep 15, 2023 • edited Loading

changeset-bot bot commented Sep 15, 2023 • edited Loading

🦋 Changeset detected

RamIdeas Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobMGEvans left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrbbot Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas commented Sep 15, 2023 •

edited

Loading

changeset-bot bot commented Sep 15, 2023 •

edited

Loading

RamIdeas Sep 15, 2023 •

edited

Loading

JacobMGEvans left a comment •

edited

Loading

RamIdeas Sep 15, 2023 •

edited

Loading

mrbbot Oct 6, 2023 •

edited

Loading

RamIdeas Sep 15, 2023 •

edited

Loading

RamIdeas Sep 15, 2023 •

edited

Loading