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

Abort signalling #732

Merged
merged 2 commits into from
Apr 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions .changeset/wet-tables-pretend.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
"wrangler": patch
---

fix: abort async operations in the `Remote` component to avoid unwanted side-effects
When the `Remote` component is unmounted, we now signal outstanding `fetch()` requests, and
`waitForPortToBeAvailable()` tasks to cancel them. This prevents unexpected requests from appearing
after the component has been unmounted, and also allows the process to exit cleanly without a delay.

fixes #375
21 changes: 14 additions & 7 deletions packages/wrangler/src/create-worker-preview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ export interface CfPreviewToken {
*/
async function sessionToken(
account: CfAccount,
ctx: CfWorkerContext
ctx: CfWorkerContext,
abortSignal: AbortSignal
): Promise<CfPreviewToken> {
const { accountId } = account;
const initUrl = ctx.zone
Expand All @@ -66,7 +67,7 @@ async function sessionToken(

const { exchange_url } = await fetchResult<{ exchange_url: string }>(initUrl);
const { inspector_websocket, token } = (await (
await fetch(exchange_url)
await fetch(exchange_url, { signal: abortSignal })
).json()) as { inspector_websocket: string; token: string };
const { host } = new URL(inspector_websocket);
const query = `cf_workers_preview_token=${token}`;
Expand Down Expand Up @@ -96,11 +97,13 @@ function randomId(): string {
async function createPreviewToken(
account: CfAccount,
worker: CfWorkerInit,
ctx: CfWorkerContext
ctx: CfWorkerContext,
abortSignal: AbortSignal
): Promise<CfPreviewToken> {
const { value, host, inspectorUrl, prewarmUrl } = await sessionToken(
account,
ctx
ctx,
abortSignal
);

const { accountId } = account;
Expand Down Expand Up @@ -155,10 +158,14 @@ async function createPreviewToken(
export async function createWorkerPreview(
init: CfWorkerInit,
account: CfAccount,
ctx: CfWorkerContext
ctx: CfWorkerContext,
abortSignal: AbortSignal
): Promise<CfPreviewToken> {
const token = await createPreviewToken(account, init, ctx);
const response = await fetch(token.prewarmUrl.href, { method: "POST" });
const token = await createPreviewToken(account, init, ctx, abortSignal);
const response = await fetch(token.prewarmUrl.href, {
method: "POST",
signal: abortSignal,
});
if (!response.ok) {
// console.error("worker failed to prewarm: ", response.statusText);
}
Expand Down
3 changes: 2 additions & 1 deletion packages/wrangler/src/dev/dev.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ function useHotkeys(
) {
// UGH, we should put port in context instead
const [toggles, setToggles] = useState(initial);
const { exit } = useApp();
useInput(
async (
input,
Expand Down Expand Up @@ -385,7 +386,7 @@ function useHotkeys(
// shut down
case "q":
case "x":
process.exit(0);
exit();
break;
default:
// nothing?
Expand Down
8 changes: 7 additions & 1 deletion packages/wrangler/src/dev/local.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,16 @@ function useLocalWorker({
const removeSignalExitListener = useRef<() => void>();
const [inspectorUrl, setInspectorUrl] = useState<string | undefined>();
useEffect(() => {
const abortController = new AbortController();
async function startLocalWorker() {
if (!bundle || !format) return;

// port for the worker
await waitForPortToBeAvailable(port, { retryPeriod: 200, timeout: 2000 });
await waitForPortToBeAvailable(port, {
retryPeriod: 200,
timeout: 2000,
abortSignal: abortController.signal,
});

if (publicDirectory) {
throw new Error(
Expand Down Expand Up @@ -249,6 +254,7 @@ function useLocalWorker({
});

return () => {
abortController.abort();
if (local.current) {
console.log("⎔ Shutting down local server.");
local.current?.kill();
Expand Down
8 changes: 7 additions & 1 deletion packages/wrangler/src/dev/remote.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export function useWorker(props: {
const startedRef = useRef(false);

useEffect(() => {
const abortController = new AbortController();
async function start() {
setToken(undefined); // reset token in case we're re-running

Expand Down Expand Up @@ -173,7 +174,8 @@ export function useWorker(props: {
accountId,
apiToken,
},
{ env: props.env, legacyEnv: props.legacyEnv, zone: props.zone }
{ env: props.env, legacyEnv: props.legacyEnv, zone: props.zone },
abortController.signal
)
);
}
Expand All @@ -182,6 +184,10 @@ export function useWorker(props: {
// since it could recover after the developer fixes whatever's wrong
console.error("remote worker:", err);
});

return () => {
abortController.abort();
};
}, [
name,
bundle,
Expand Down
9 changes: 8 additions & 1 deletion packages/wrangler/src/inspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,19 +154,26 @@ export default function useInspector(props: InspectorProps) {
* of the component lifecycle. Convenient.
*/
useEffect(() => {
const abortController = new AbortController();
async function startInspectorProxy() {
await waitForPortToBeAvailable(props.port, {
retryPeriod: 200,
timeout: 2000,
abortSignal: abortController.signal,
});
server.listen(props.port);
}
startInspectorProxy().catch((err) => console.error(err));
startInspectorProxy().catch((err) => {
if ((err as { code: string }).code !== "ABORT_ERR") {
console.error(err);
}
});
return () => {
server.close();
// Also disconnect any open websockets/devtools connections
wsServer.clients.forEach((ws) => ws.close());
wsServer.close();
abortController.abort();
};
}, [props.port, server, wsServer]);

Expand Down
21 changes: 18 additions & 3 deletions packages/wrangler/src/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,21 +289,29 @@ export function usePreviewServer({
// Start/stop the server whenever the
// containing component is mounted/unmounted.
useEffect(() => {
const abortController = new AbortController();
if (proxyServer === undefined) {
return;
}

waitForPortToBeAvailable(port, { retryPeriod: 200, timeout: 2000 })
waitForPortToBeAvailable(port, {
retryPeriod: 200,
timeout: 2000,
abortSignal: abortController.signal,
})
.then(() => {
proxyServer.listen(port, ip);
console.log(`⬣ Listening at ${localProtocol}://${ip}:${port}`);
})
.catch((err) => {
console.error(`⬣ Failed to start server: ${err}`);
if ((err as { code: string }).code !== "ABORT_ERR") {
console.error(`⬣ Failed to start server: ${err}`);
}
});

return () => {
proxyServer.close();
abortController.abort();
};
}, [port, ip, proxyServer, localProtocol]);
}
Expand Down Expand Up @@ -398,9 +406,16 @@ function createStreamHandler(
*/
export async function waitForPortToBeAvailable(
port: number,
options: { retryPeriod: number; timeout: number }
options: { retryPeriod: number; timeout: number; abortSignal: AbortSignal }
): Promise<void> {
return new Promise((resolve, reject) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(options.abortSignal as any).addEventListener("abort", () => {
const abortError = new Error("waitForPortToBeAvailable() aborted");
(abortError as Error & { code: string }).code = "ABORT_ERR";
doReject(abortError);
});

const timeout = setTimeout(() => {
doReject(new Error(`Timed out waiting for port ${port}`));
}, options.timeout);
Expand Down