Abort signalling #732

JacobMGEvans · 2022-03-30T17:45:08Z

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

changeset-bot · 2022-03-30T17:45:11Z

🦋 Changeset detected

Latest commit: 89edf75

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

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

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

github-actions · 2022-03-30T17:46:03Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2091243209/npm-package-wrangler-732

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/732/npm-package-wrangler-732

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2091243209/npm-package-wrangler-732 dev path/to/script.js

packages/wrangler/src/proxy.ts

packages/wrangler/src/dev/remote.tsx

packages/wrangler/src/create-worker-preview.ts

threepointone · 2022-03-31T08:39:54Z

packages/wrangler/src/proxy.ts

@@ -421,6 +427,9 @@ export async function waitForPortToBeAvailable(
    }

    function checkPort() {
+      // Will clear the interval and timeout if signal is called,
+      // using resolve prevents Error messages on the console.
+      options.abortCtrl?.aborted && doResolve();


let's wrap this in a conditional

Suggested change

options.abortCtrl?.aborted && doResolve();

if(options.abortCtrl?.aborted){

doResolve();

}

Probably more clear, will do.

petebacondarwin

I agree with @threepointone's suggestions; but otherwise LGTM.

petebacondarwin · 2022-03-31T10:36:11Z

.changeset/wet-tables-pretend.md

+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`


Suggested change

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`

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.

Much better wording, thank you!!

petebacondarwin

Actually I wonder if the waitForPortToBeAvailable() abort should be more along the lines of:

export async function waitForPortToBeAvailable(
  port: number,
  options: { retryPeriod: number; timeout: number; abortCtrl?: AbortSignal }
): Promise<void> {
  return new Promise((resolve, reject) => {
    abortSignal.onabort(() => {
      const abortError = new Error("");
      (error as {code: string}).code = "ABORT_ERR";
      reject(abortError);
    });

threepointone · 2022-03-31T19:32:17Z

packages/wrangler/src/proxy.ts

 ): Promise<void> {
  return new Promise((resolve, reject) => {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    (options.abortSignal as any).addEventListener("abort", () => {
+      const error = new Error("");


This will log "Failed to start server:" which will be confusing to anyone who reads it. Could we add a code or something this error, and read it on line 307 and not log anything if it's an aborted attempt? Like @petebacondarwin described in #732 (review)

Suggested change

const error = new Error("");

const abortError = new Error("waitForPortToBeAvailable() aborted");

(error as {code: string}).code = "ABORT_ERR";

petebacondarwin

Just that one change - otherwise LGTM!

petebacondarwin · 2022-04-04T16:30:57Z

packages/wrangler/src/proxy.ts

 ): Promise<void> {
  return new Promise((resolve, reject) => {
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    (options.abortSignal as any).addEventListener("abort", () => {
+      const error = new Error("");


Suggested change

const error = new Error("");

const abortError = new Error("waitForPortToBeAvailable() aborted");

(error as {code: string}).code = "ABORT_ERR";

…e `useEffect` the abort signal is called in the return which allows for graceful closing with no hanging of the `waitForPortToBeAvailable` fixes #375

JacobMGEvans added the bug label Mar 30, 2022

JacobMGEvans requested a review from petebacondarwin March 30, 2022 17:45

JacobMGEvans requested a review from threepointone as a code owner March 30, 2022 17:45

threepointone reviewed Mar 31, 2022

View reviewed changes

packages/wrangler/src/create-worker-preview.ts Outdated Show resolved Hide resolved

threepointone reviewed Mar 31, 2022

View reviewed changes

petebacondarwin reviewed Mar 31, 2022

View reviewed changes

petebacondarwin requested changes Mar 31, 2022

View reviewed changes

JacobMGEvans force-pushed the abort-signalling branch from 89b464c to c0eba4c Compare March 31, 2022 16:56

JacobMGEvans requested review from petebacondarwin and threepointone March 31, 2022 16:57

threepointone reviewed Mar 31, 2022

View reviewed changes

fix: abort some of the fetches when we unmount the Remote component

e323c2c

JacobMGEvans force-pushed the abort-signalling branch from c0eba4c to 53d0589 Compare April 4, 2022 15:54

petebacondarwin requested changes Apr 4, 2022

View reviewed changes

Adding abort to the waitForPortToBeAvailable as an option and in th…

89edf75

…e `useEffect` the abort signal is called in the return which allows for graceful closing with no hanging of the `waitForPortToBeAvailable` fixes #375

JacobMGEvans force-pushed the abort-signalling branch from 53d0589 to 89edf75 Compare April 4, 2022 16:37

petebacondarwin approved these changes Apr 4, 2022

View reviewed changes

JacobMGEvans merged commit c63ea3d into main Apr 4, 2022

JacobMGEvans deleted the abort-signalling branch April 4, 2022 16:43

github-actions bot mentioned this pull request Apr 4, 2022

Version Packages #721

Merged

petebacondarwin mentioned this pull request May 10, 2022

🐛 BUG: wrangler dev still sometimes fails to exit cleanly after Ctl-C #958

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abort signalling #732

Abort signalling #732

JacobMGEvans commented Mar 30, 2022 •

edited

Loading

changeset-bot bot commented Mar 30, 2022 •

edited

Loading

github-actions bot commented Mar 30, 2022 •

edited

Loading

threepointone Mar 31, 2022

JacobMGEvans Mar 31, 2022

petebacondarwin left a comment

petebacondarwin Mar 31, 2022

JacobMGEvans Mar 31, 2022

petebacondarwin left a comment

threepointone Mar 31, 2022

petebacondarwin Apr 4, 2022

petebacondarwin left a comment

petebacondarwin Apr 4, 2022

-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`
+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.

	const error = new Error("");
	const abortError = new Error("waitForPortToBeAvailable() aborted");
	(error as {code: string}).code = "ABORT_ERR";

Abort signalling #732

Abort signalling #732

Conversation

JacobMGEvans commented Mar 30, 2022 • edited Loading

changeset-bot bot commented Mar 30, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Mar 30, 2022 • edited Loading

threepointone Mar 31, 2022

Choose a reason for hiding this comment

JacobMGEvans Mar 31, 2022

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Mar 31, 2022

Choose a reason for hiding this comment

JacobMGEvans Mar 31, 2022

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

threepointone Mar 31, 2022

Choose a reason for hiding this comment

petebacondarwin Apr 4, 2022

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Apr 4, 2022

Choose a reason for hiding this comment

JacobMGEvans commented Mar 30, 2022 •

edited

Loading

changeset-bot bot commented Mar 30, 2022 •

edited

Loading

github-actions bot commented Mar 30, 2022 •

edited

Loading