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-dev/vite): don't trigger request abort signal on dev server response close #8130

Merged

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 25, 2023

Follow up #8062

In the previous PR, I blindly copied the code around AbortController from packages/remix-express/server.ts, but the way chaining "response close" to "request abort" might have unintended issues.

I was testing entry.server.cloudflare.tsx on vite dev and I expected it to work on node 18, but I found that it produces a following error log for each request (though response succeeds and the app is still functional). I initially thought this is due to installGlobals, but I saw a similar log even without it.

TypeError: The stream has already been closed; do not close it again!
    at ReadableByteStreamController.close (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected]/node_modules/web-streams-polyfill/dist/ponyfill.js:853:23)
    at close (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.browser.development.js:143:15)
    at flushCompletedQueues (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.browser.development.js:6895:9)
    at abort (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.browser.development.js:6940:7)
    at EventTarget.listener (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.browser.development.js:6989:9)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:737:20)
    at EventTarget.dispatchEvent (node:internal/event_target:679:26)
    at abortSignal (node:internal/abort_controller:314:10)
    at AbortController.abort (node:internal/abort_controller:344:5)
    at ServerResponse.<anonymous> (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/@[email protected]_@[email protected]_mti6rk27yzoho5bnzwyf7xqtii/node_modules/@remix-run/dev/dist/vite/node/adapter.js:45:36)
And logs without "installGlobals"
TypeError [ERR_INVALID_STATE]: Invalid state: Controller is already closed
    at new NodeError (node:internal/errors:399:5)
    at ReadableByteStreamController.close (node:internal/webstreams/readablestream:1127:13)
    at close (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.browser.development.js:143:15)
    at flushCompletedQueues (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.browser.development.js:6895:9)
    at abort (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.browser.development.js:6940:7)
    at EventTarget.listener (/home/hiroshi/code/tmp/remix-vite-deploy/node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom-server.browser.development.js:6989:9)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:737:20)
    at EventTarget.dispatchEvent (node:internal/event_target:679:26)
    at abortSignal (node:internal/abort_controller:314:10)
    at AbortController.abort (node:internal/abort_controller:344:5) {
  code: 'ERR_INVALID_STATE'
}

This seems to be due to stream close event circling back by the following chain response body ReadableStream close -> node http ServerResponse close -> AbortController.abort -> response body ReadableStream close where 3rd (the last) chain is coming from:

const body = await renderToReadableStream(
<RemixServer context={remixContext} url={request.url} />,
{
signal: request.signal,

I think 2nd chain is the unnecessary one and I'm thinking to remove it since this one was not also present in the version before #8062 which was based on solit-start.

testing strategy

I verified the log disappears after applying a patch in hi-ogawa/remix-vite-deployment-examples#7
I tested both with and without installGlobals.

Copy link

changeset-bot bot commented Nov 25, 2023

⚠️ No Changeset found

Latest commit: 6852971

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@hi-ogawa hi-ogawa changed the title fix(remix-dev/vite): remove request abort on response close fix(remix-dev/vite): don't trigger request abort signal on response close Nov 25, 2023
@hi-ogawa hi-ogawa marked this pull request as ready for review November 26, 2023 01:49
hi-ogawa and others added 4 commits November 26, 2023 10:51
this is a fix for an unreleased feature, so it's covered by the existing changeset
@markdalgleish markdalgleish changed the title fix(remix-dev/vite): don't trigger request abort signal on response close fix(remix-dev/vite): don't trigger request abort signal on dev server response close Nov 26, 2023
@markdalgleish markdalgleish merged commit e11f479 into remix-run:dev Nov 26, 2023
9 checks passed
Copy link
Contributor

github-actions bot commented Dec 5, 2023

🤖 Hello there,

We just published version 2.4.0-pre.0 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!

Copy link
Contributor

🤖 Hello there,

We just published version 2.4.0 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants