Skip to content

Commit

Permalink
Abort signalling (#732)
Browse files Browse the repository at this point in the history
* fix: abort some of the fetches when we unmount the `Remote` component

* Adding abort to the `waitForPortToBeAvailable` as an option and in the `useEffect` the abort signal is called in the return which allows for graceful closing with no hanging
of the `waitForPortToBeAvailable`

fixes #375

Co-authored-by: Pete Bacon Darwin <[email protected]>
  • Loading branch information
JacobMGEvans and petebacondarwin authored Apr 4, 2022
1 parent 91873e4 commit c63ea3d
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 14 deletions.
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

0 comments on commit c63ea3d

Please sign in to comment.