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

fix(remix-express,remix-vercel): fix request.signal #3626

Merged
merged 8 commits into from Aug 29, 2022

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Jun 30, 2022

This PR fixes the signal we assign to our Fetch Request in Express + Vercel. We used to abort the signal on the close event on the Express/Vercel Request, but in POST's this happens to be as soon as we've consumed the body, so any async flow in an action causes the signal to be aborted:

export async function action({ request }) {
    await new Promise(r => setTimeout(r, 0));
    console.log(request.signal.aborted); // This is true every time
    ...
}

Instead we should we aborting our server-side processing of the request as soon as we are no longer able to send a Response, so we can listen for the Express/Vercel Response close event instead.

Open Questions:

  1. I added an integration test to assert that the signal is not immediately aborted, but curious if we have a way to assert that it's correctly aborted if the response closes?
    • Jacob and I manually tested this by sending a fetch request from the browser and manually triggering its AbortController - and then checking the console logs on the Remix server. Unsure if we can get at that level of info via our integration harness?
  2. Other runtimes
    • CF + Deno provide a signal on the incoming request so we rely on them to trigger abort the signal properly on their end
    • Arc/Netlify don't seem to have any signal available, and we do not create one so request.signal == null inside our Remix code. This will potentially be problematic when we pull in the new router since it will be relying on the existing signal. We can either just make our references defensive via request.signal?.aborted, but it would be better if we could find some way to wire up a legit signal?
      • I chose to create a signal (which will never be aborted) for these and added an invariant(request.signal) in createStaticHandler.query to better align with the Request typings

req.on("close", () => {
controller.abort();
});
res.on("close", () => controller.abort());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Abort on response close instead of request close

expect(await app.getHtml(".loader")).toMatch("false");

await app.clickElement('button[type="submit"]');
expect(await app.getHtml(".action")).toMatch("false");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prior to the fix this would be true

@brophdawg11
Copy link
Contributor Author

Hm - seems to be failing CI on a yarn install - will try kicking off a new build tomorrow:

error An unexpected error occurred: "https://registry.yarnpkg.com/@types/eslint/-/eslint-8.4.4.tgz: Request failed \"404 Not Found\"".

@changeset-bot
Copy link

changeset-bot bot commented Aug 25, 2022

🦋 Changeset detected

Latest commit: 70ca93c

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/vercel Patch
@remix-run/serve Patch
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/server-runtime 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

@MichaelDeBoey MichaelDeBoey changed the title fix: fix request.signal for express/vercel fix(remix-express,remix-vercel): fix request.signal Aug 25, 2022
@brophdawg11 brophdawg11 merged commit 81ec1b7 into dev Aug 29, 2022
@brophdawg11 brophdawg11 deleted the brophdawg11/abortsignal branch August 29, 2022 14:29
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-81ec1b7-20220830 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants