Skip to content

[wrangler] fix: prevent repeated reloads with circular service bindings #4699

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.

Sign up for GitHub

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

Merged
mrbbot merged 1 commit into from
Jan 5, 2024
Merged

[wrangler] fix: prevent repeated reloads with circular service bindings #4699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/gorgeous-turkeys-wait.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"wrangler": patch
---

fix: prevent repeated reloads with circular service bindings

`[email protected]` introduced a bug where starting multiple `wrangler dev` sessions with service bindings to each other resulted in a reload loop. This change ensures Wrangler only reloads when dependent `wrangler dev` sessions start/stop.
2 changes: 2 additions & 0 deletions packages/wrangler/src/api/startDevWorker/events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { CfDurableObject } from "../../deployment-bundle/worker";
import type { EsbuildBundle } from "../../dev/use-esbuild";
import type { DevToolsEvent } from "./devtools";
import type { StartDevWorkerOptions } from "./types";
Expand Down Expand Up @@ -147,4 +148,5 @@ export type ProxyData = {
headers: Record<string, string | undefined>;
liveReload?: boolean;
proxyLogsToController?: boolean;
internalDurableObjects?: CfDurableObject[];
};
10 changes: 9 additions & 1 deletion packages/wrangler/src/dev/dev.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { logger } from "../logger";
import openInBrowser from "../open-in-browser";
import { getWranglerTmpDir } from "../paths";
import { openInspector } from "./inspect";
import { Local } from "./local";
import { Local, maybeRegisterLocalWorker } from "./local";
import { Remote } from "./remote";
import { useEsbuild } from "./use-esbuild";
import { validateDevProps } from "./validate-dev-props";
Expand Down Expand Up @@ -387,6 +387,14 @@ function DevSession(props: DevSessionProps) {
finalIp = url.hostname;
finalPort = parseInt(url.port);

if (props.local) {
await maybeRegisterLocalWorker(
url,
props.name,
proxyData.internalDurableObjects
);
}

if (process.send) {
process.send(
JSON.stringify({
Expand Down
29 changes: 18 additions & 11 deletions packages/wrangler/src/dev/local.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,15 @@ import { MiniflareServer } from "./miniflare";
import { DEFAULT_WORKER_NAME } from "./miniflare";
import type { ProxyData } from "../api";
import type { Config } from "../config";
import type { CfWorkerInit, CfScriptFormat } from "../deployment-bundle/worker";
import type {
CfWorkerInit,
CfScriptFormat,
CfDurableObject,
} from "../deployment-bundle/worker";
import type { WorkerRegistry } from "../dev-registry";
import type { EnablePagesAssetsServiceBindingOptions } from "../miniflare-cli/types";
import type { AssetPaths } from "../sites";
import type { ConfigBundle, ReloadedEvent } from "./miniflare";
import type { ConfigBundle } from "./miniflare";
import type { EsbuildBundle } from "./use-esbuild";

export interface LocalProps {
Expand Down Expand Up @@ -95,24 +99,28 @@ export async function localPropsToConfigBundle(
};
}

export function maybeRegisterLocalWorker(event: ReloadedEvent, name?: string) {
export function maybeRegisterLocalWorker(
url: URL,
name?: string,
internalDurableObjects?: CfDurableObject[]
) {
if (name === undefined) return;

let protocol = event.url.protocol;
protocol = protocol.substring(0, event.url.protocol.length - 1);
let protocol = url.protocol;
protocol = protocol.substring(0, url.protocol.length - 1);
if (protocol !== "http" && protocol !== "https") return;

const port = parseInt(event.url.port);
const port = parseInt(url.port);
return registerWorker(name, {
protocol,
mode: "local",
port,
host: event.url.hostname,
durableObjects: event.internalDurableObjects.map((binding) => ({
host: url.hostname,
durableObjects: (internalDurableObjects ?? []).map((binding) => ({
name: binding.name,
className: binding.class_name,
})),
durableObjectsHost: event.url.hostname,
durableObjectsHost: url.hostname,
durableObjectsPort: port,
});
}
Expand Down Expand Up @@ -157,8 +165,6 @@ function useLocalWorker(props: LocalProps) {
const newServer = new MiniflareServer();
miniflareServerRef.current = server = newServer;
server.addEventListener("reloaded", async (event) => {
await maybeRegisterLocalWorker(event, props.name);
Copy link
Contributor

@petebacondarwin petebacondarwin Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the crux of this fix, right?

Copy link
Contributor Author

@mrbbot mrbbot Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda, we still register the local worker, just with the URL of the proxy worker, not the user worker. The proxy worker has a stable URL across reloads, breaking the reload loop.


const proxyData: ProxyData = {
userWorkerUrl: {
protocol: event.url.protocol,
Expand All @@ -185,6 +191,7 @@ function useLocalWorker(props: LocalProps) {
// in local mode, the logs are already being printed to the console by workerd but only for workers written in "module" format
// workers written in "service-worker" format still need to proxy logs to the ProxyController
proxyLogsToController: props.format === "service-worker",
internalDurableObjects: event.internalDurableObjects,
};

props.onReady?.(
Expand Down
9 changes: 7 additions & 2 deletions packages/wrangler/src/dev/start-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,12 @@ export async function startDevServer(
ip = url.hostname;
port = parseInt(url.port);

await maybeRegisterLocalWorker(
url,
props.name,
proxyData.internalDurableObjects
);

props.onReady?.(ip, port, proxyData);

// temp: fake these events by calling the handler directly
Expand Down Expand Up @@ -393,8 +399,6 @@ export async function startLocalServer(
return new Promise<{ stop: () => Promise<void> }>((resolve, reject) => {
const server = new MiniflareServer();
server.addEventListener("reloaded", async (event) => {
await maybeRegisterLocalWorker(event, props.name);

const proxyData: ProxyData = {
userWorkerUrl: {
protocol: event.url.protocol,
Expand All @@ -420,6 +424,7 @@ export async function startLocalServer(
// in local mode, the logs are already being printed to the console by workerd but only for workers written in "module" format
// workers written in "service-worker" format still need to proxy logs to the ProxyController
proxyLogsToController: props.format === "service-worker",
internalDurableObjects: event.internalDurableObjects,
};

props.onReady?.(event.url.hostname, parseInt(event.url.port), proxyData);
Expand Down