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

Cannot cancel a stream that already has a reader #3971

Open
jdnichollsc opened this issue Aug 10, 2022 · 21 comments
Open

Cannot cancel a stream that already has a reader #3971

jdnichollsc opened this issue Aug 10, 2022 · 21 comments
Labels
bug:unverified feat:fetch Issues related to @remix-run/web-fetch

Comments

@jdnichollsc
Copy link

jdnichollsc commented Aug 10, 2022

What version of Remix are you using?

1.6.7

Steps to Reproduce

I'm executing a http request from a route Action but I'm getting this error because Server is busy (not responding):

[frontend] /frontend/node_modules/web-streams-polyfill/src/lib/readable-stream.ts:149
[frontend]       return promiseRejectedWith(new TypeError('Cannot cancel a stream that already has a reader'));
[frontend]                                  ^
[frontend] TypeError: Cannot cancel a stream that already has a reader
[frontend]     at ReadableStream.cancel (/frontend/node_modules/web-streams-polyfill/src/lib/readable-stream.ts:149:34)
[frontend]     at abort (/frontend/node_modules/@remix-run/web-fetch/src/fetch.js:75:18)
[frontend]     at AbortSignal.abortAndFinalize (/frontend/node_modules/@remix-run/web-fetch/src/fetch.js:91:4)
[frontend]     at AbortSignal.[nodejs.internal.kHybridDispatch] (node:internal/event_target:643:20)
[frontend]     at AbortSignal.dispatchEvent (node:internal/event_target:585:26)
[frontend]     at abortSignal (node:internal/abort_controller:284:10)
[frontend]     at AbortController.abort (node:internal/abort_controller:315:5)
[frontend]     at Timeout._onTimeout (/frontend/apps/webapp/app/utils/http.ts:16:42)
[frontend]     at listOnTimeout (node:internal/timers:559:17)
[frontend]     at processTimers (node:internal/timers:502:7)

I created a simple utility to support timeout with fetch:

const DEFAULT_TIMEOUT = 5000;

export type HttpOptions = RequestInit & {
  timeout?: number;
  defaultResponse?: unknown;
  parseJSON?: boolean;
}

async function httpRequest(
  resource: string,
  options: HttpOptions = { timeout: DEFAULT_TIMEOUT, parseJSON: true },
) {
  const { timeout, ...rest } = options;
  
  const controller = new AbortController();
  const id = setTimeout(() => controller.abort(), timeout || DEFAULT_TIMEOUT);
  try {
    const response = await fetch(resource, {
      ...rest,
      signal: controller.signal
    });
    clearTimeout(id);
    return rest.parseJSON === false ? response : response.json();
  } catch (error) {
    if (rest.defaultResponse !== undefined) {
      return rest.defaultResponse;
    }
    if (controller.signal.aborted) {
      throw new Error('Server is busy, please try again');
    }
    throw error;
  }
}

export { httpRequest };

Then I'm using that method to execute a POST request:

export const action: ActionFunction = async ({ request }) => {
  const formData = await request.formData();

  const email = formData.get('email');
  try {
    await httpRequest(`${apiURL}/auth`, {
      method: 'POST',
      body: JSON.stringify({
        email,
      }),
      parseJSON: false,
    });

    // ...
  } catch (error) {
    return {
      error: (error as Error)?.message,
    }
  }
};

Also I'm already using CatchBoundary and ErrorBoundary for this page:

function renderLoginPage(error?: Error) {
  const data = useLoaderData();
  return (
    <div id="main-content" className="relative pb-36">
      <LoginPage {...data} error={error} />
    </div>
  );
}

export const CatchBoundary = () => {
  const caught = useCatch();
  return renderLoginPage(caught.data);
};

export const ErrorBoundary: ErrorBoundaryComponent = ({ error }) => {
  return renderLoginPage(error);
};

export default function () {
  return renderLoginPage();
}

Expected Behavior

Be able to get the error message using useActionData Hook from Remix, it's working for other validations before executing the HTTP request.

Actual Behavior

Getting an unexpected exception:

Remix error

Thanks for your help! <3

@MisteryPoints
Copy link

MisteryPoints commented Aug 12, 2022

I have similar issue, but not the same. "TypeError: This stream has already been locked for exclusive reading by another reader"

Im trying to request text from Form Data and Files too. But Form has 2 different methods, but not possible to read the request with both of them. Sorry if my code isnt efficent enough but im trying.

export async function action(props) {
    const {request} = props 
    const formDatas = request.formData()
    const  data =  {
      title: formDatas.get('title'),
      slug: formDatas.get('slug')
    } 
    await createCategory(data)  
    const uploadHandler = unstable_createFileUploadHandler({
      maxPartSize: 10_000_000,
      directory: path.join(__dirname, '..', `public/uploads`),
      file: ({ filename }) => filename,
    })
    const formData = await unstable_parseMultipartFormData(
      request,
      uploadHandler
    ) 
    const image = formData.get('image') 
    
    if (image) {
      console.log(`File uploaded to server/public/uploads/${image.name}`)
    } else {
      console.log("No file uploaded");
    } 
    return redirect('/')
}

@itsanirbanroy
Copy link

@MisteryPoints Try cloning the request object, and read data separately from that cloned object. Even I faced the same issue, and have solved it by following this approach -

export async function action(props) {
    const {request} = props 
    const clonedData = request.clone()
    const formDatas = await clonedData.formData()
    const  data =  {
      title: formDatas.get('title'),
      slug: formDatas.get('slug')
    } 
    await createCategory(data)  
    const uploadHandler = unstable_createFileUploadHandler({
      maxPartSize: 10_000_000,
      directory: path.join(__dirname, '..', `public/uploads`),
      file: ({ filename }) => filename,
    })
    const formData = await unstable_parseMultipartFormData(
      request,
      uploadHandler
    ) 
    const image = formData.get('image') 
    
    if (image) {
      console.log(`File uploaded to server/public/uploads/${image.name}`)
    } else {
      console.log("No file uploaded");
    } 
    return redirect('/')
}

@MisteryPoints
Copy link

@itsanirbanroy Thanks bro!, i did the same before in Aug 12, when i readed a bit of request documentation.

@jdnichollsc
Copy link
Author

jdnichollsc commented Sep 13, 2022

It's working great from the loader of the routes with RemixJS 1.7.0, but I'm getting this error with a POST HTTP request from a simple action:

export const action: ActionFunction = async ({ request }) => {
  const formData = await request.formData();
  const email = formData.get('email')?.toString();
  try {
    await httpRequest(`${apiURL}/auth`, {
      method: 'POST',
      body: JSON.stringify({
        email,
      }),
      parseJSON: false,
      timeout: 1000,
    });

    return redirect('/login');
  } catch (error) {
    return {
      error: (error as Error)?.message,
    };
  }
};

Output:

/node_modules/web-streams-polyfill/src/lib/readable-stream.ts:149
      return promiseRejectedWith(new TypeError('Cannot cancel a stream that already has a reader'));
                                 ^
TypeError: Cannot cancel a stream that already has a reader
    at ReadableStream.cancel (/node_modules/web-streams-polyfill/src/lib/readable-stream.ts:149:34)
    at abort (/node_modules/@remix-run/web-fetch/src/fetch.js:75:18)
    at AbortSignal.abortAndFinalize (/node_modules/@remix-run/web-fetch/src/fetch.js:91:4)
    at AbortSignal.[nodejs.internal.kHybridDispatch] (node:internal/event_target:694:20)
    at AbortSignal.dispatchEvent (node:internal/event_target:636:26)
    at abortSignal (node:internal/abort_controller:292:10)
    at AbortController.abort (node:internal/abort_controller:323:5)
    at Timeout._onTimeout (/apps/webapp/app/utils/http.ts:21:42)
    at listOnTimeout (node:internal/timers:559:17)
    at processTimers (node:internal/timers:502:7)

App Error

Please let me know and thanks for your help!

@aniravi24
Copy link

aniravi24 commented Feb 14, 2023

I see this error when canceling a fetch request with AbortController. When I use remix's fetch library with graphql-request instead of the default node-fetch that it uses, I run into this sometimes when calling controller.abort()

For now, I decided not to use Remix's custom fetch library for that purpose.

@bmaycc
Copy link

bmaycc commented Mar 12, 2023

I also ran into this as well. For now, as a workaround (to @aniravi24's point of avoiding the web-fetch usage), I did the following:

  • added node-fetch 2 to packages.json (since node-fetch 3 is ESM only)
"node-fetch": "^2",
  • Then add the following to a fetchOverlay.server.ts file
const nodeFetch = require("node-fetch");
let fetchOverlay = false;
export function setupFetch() {
  if (!fetchOverlay) {
    globalThis.fetch = nodeFetch as any;
    fetchOverlay = true;
  }
}
  • Call setupFetch() somewhere common (or in root)

@MEBonham
Copy link

MEBonham commented Apr 9, 2023

This still seems to be an active issue, I just ran into it too.

@kayac-chang
Copy link
Contributor

Also facing this issue, the following is my investigation

I think this issue can't be solve by clone the request,
it is when request aborted will throw an error which even can't be catch from user side,
so the server crash by this unhandled error.

  1. remix-run/node overwrite the default fetch under the hood,

    global.fetch = nodeFetch as typeof fetch;

  2. the implementation in remix-run/web-std-io could be found at
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/src/fetch.js#L39

  3. the issue is
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/src/fetch.js#L75

  4. this error is throw by underline polyfill readable stream,
    seems like it will throw error if the response body has been locked (because it will be consume by http request).
    https://github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/readable-stream.ts#L149

  5. seems like the test case didn't do a good job,
    the controller abort the request before it actually fired,
    the request.body is not be consume by reader yet.
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/test/main.js#L1129

the test case below try to defer the controller abort,
after the request.body be consume.

diff --git a/packages/fetch/src/fetch.js b/packages/fetch/src/fetch.js
index fcbc379..6a46f77 100644
--- a/packages/fetch/src/fetch.js
+++ b/packages/fetch/src/fetch.js
@@ -72,6 +72,8 @@ async function fetch(url, options_ = {}) {
                        const error = new AbortError('The operation was aborted.');
                        reject(error);
                        if (request.body) {
+                               console.log('request.body', request.body)
+                               console.log('request.body.locked', request.body.locked)
                                request.body.cancel(error);
                        }

diff --git a/packages/fetch/test/main.js b/packages/fetch/test/main.js
index cc02728..0b7d95f 100644
--- a/packages/fetch/test/main.js
+++ b/packages/fetch/test/main.js
@@ -1126,7 +1126,9 @@ describe('node-fetch', () => {
                                                .and.have.property('name', 'AbortError')
                                ]);

-                               controller.abort();
+                               setTimeout(() => {
+                                       controller.abort();
+                               }, 10)

                                return result;
                        });

I didn't solve this issue, need other people input.

@kayac-chang
Copy link
Contributor

Also facing this issue, the following is my investigation

I think this issue can't be solve by clone the request, it is when request aborted will throw an error which even can't be catch from user side, so the server crash by this unhandled error.

  1. remix-run/node overwrite the default fetch under the hood,
    global.fetch = nodeFetch as typeof fetch;
  2. the implementation in remix-run/web-std-io could be found at
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/src/fetch.js#L39
  3. the issue is
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/src/fetch.js#L75
  4. this error is throw by underline polyfill readable stream,
    seems like it will throw error if the response body has been locked (because it will be consume by http request).
    https://github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/readable-stream.ts#L149
  5. seems like the test case didn't do a good job,
    the controller abort the request before it actually fired,
    the request.body is not be consume by reader yet.
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/test/main.js#L1129

the test case below try to defer the controller abort, after the request.body be consume.

diff --git a/packages/fetch/src/fetch.js b/packages/fetch/src/fetch.js
index fcbc379..6a46f77 100644
--- a/packages/fetch/src/fetch.js
+++ b/packages/fetch/src/fetch.js
@@ -72,6 +72,8 @@ async function fetch(url, options_ = {}) {
                        const error = new AbortError('The operation was aborted.');
                        reject(error);
                        if (request.body) {
+                               console.log('request.body', request.body)
+                               console.log('request.body.locked', request.body.locked)
                                request.body.cancel(error);
                        }

diff --git a/packages/fetch/test/main.js b/packages/fetch/test/main.js
index cc02728..0b7d95f 100644
--- a/packages/fetch/test/main.js
+++ b/packages/fetch/test/main.js
@@ -1126,7 +1126,9 @@ describe('node-fetch', () => {
                                                .and.have.property('name', 'AbortError')
                                ]);

-                               controller.abort();
+                               setTimeout(() => {
+                                       controller.abort();
+                               }, 10)

                                return result;
                        });

I didn't solve this issue, need other people input.

Also, attach a repository for reproduce this issue

https://github.com/kayac-chang/remix-abort-test/tree/main

https://gitpod.io/start/#kayacchang-remixabortte-0ohsnxppt54

@aniravi24
Copy link

aniravi24 commented Apr 20, 2023

Also facing this issue, the following is my investigation
I think this issue can't be solve by clone the request, it is when request aborted will throw an error which even can't be catch from user side, so the server crash by this unhandled error.

  1. remix-run/node overwrite the default fetch under the hood,
    global.fetch = nodeFetch as typeof fetch;
  2. the implementation in remix-run/web-std-io could be found at
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/src/fetch.js#L39
  3. the issue is
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/src/fetch.js#L75
  4. this error is throw by underline polyfill readable stream,
    seems like it will throw error if the response body has been locked (because it will be consume by http request).
    https://github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/readable-stream.ts#L149
  5. seems like the test case didn't do a good job,
    the controller abort the request before it actually fired,
    the request.body is not be consume by reader yet.
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/test/main.js#L1129

the test case below try to defer the controller abort, after the request.body be consume.

diff --git a/packages/fetch/src/fetch.js b/packages/fetch/src/fetch.js
index fcbc379..6a46f77 100644
--- a/packages/fetch/src/fetch.js
+++ b/packages/fetch/src/fetch.js
@@ -72,6 +72,8 @@ async function fetch(url, options_ = {}) {
                        const error = new AbortError('The operation was aborted.');
                        reject(error);
                        if (request.body) {
+                               console.log('request.body', request.body)
+                               console.log('request.body.locked', request.body.locked)
                                request.body.cancel(error);
                        }

diff --git a/packages/fetch/test/main.js b/packages/fetch/test/main.js
index cc02728..0b7d95f 100644
--- a/packages/fetch/test/main.js
+++ b/packages/fetch/test/main.js
@@ -1126,7 +1126,9 @@ describe('node-fetch', () => {
                                                .and.have.property('name', 'AbortError')
                                ]);

-                               controller.abort();
+                               setTimeout(() => {
+                                       controller.abort();
+                               }, 10)

                                return result;
                        });

I didn't solve this issue, need other people input.

Also, attach a repository for reproduce this issue

https://github.com/kayac-chang/remix-abort-test/tree/main

https://gitpod.io/start/#kayacchang-remixabortte-0ohsnxppt54

I'm pretty sure Remix accepts PRs with a failing test case as a bug report, so if you're able to reproduce it in a test case and open a PR, that might bring this issue some attention.

@kayac-chang
Copy link
Contributor

Also facing this issue, the following is my investigation
I think this issue can't be solve by clone the request, it is when request aborted will throw an error which even can't be catch from user side, so the server crash by this unhandled error.

  1. remix-run/node overwrite the default fetch under the hood,
    global.fetch = nodeFetch as typeof fetch;
  2. the implementation in remix-run/web-std-io could be found at
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/src/fetch.js#L39
  3. the issue is
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/src/fetch.js#L75
  4. this error is throw by underline polyfill readable stream,
    seems like it will throw error if the response body has been locked (because it will be consume by http request).
    https://github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/readable-stream.ts#L149
  5. seems like the test case didn't do a good job,
    the controller abort the request before it actually fired,
    the request.body is not be consume by reader yet.
    https://github.com/remix-run/web-std-io/blob/d2a003fe92096aaf97ab2a618b74875ccaadc280/packages/fetch/test/main.js#L1129

the test case below try to defer the controller abort, after the request.body be consume.

diff --git a/packages/fetch/src/fetch.js b/packages/fetch/src/fetch.js
index fcbc379..6a46f77 100644
--- a/packages/fetch/src/fetch.js
+++ b/packages/fetch/src/fetch.js
@@ -72,6 +72,8 @@ async function fetch(url, options_ = {}) {
                        const error = new AbortError('The operation was aborted.');
                        reject(error);
                        if (request.body) {
+                               console.log('request.body', request.body)
+                               console.log('request.body.locked', request.body.locked)
                                request.body.cancel(error);
                        }

diff --git a/packages/fetch/test/main.js b/packages/fetch/test/main.js
index cc02728..0b7d95f 100644
--- a/packages/fetch/test/main.js
+++ b/packages/fetch/test/main.js
@@ -1126,7 +1126,9 @@ describe('node-fetch', () => {
                                                .and.have.property('name', 'AbortError')
                                ]);

-                               controller.abort();
+                               setTimeout(() => {
+                                       controller.abort();
+                               }, 10)

                                return result;
                        });

I didn't solve this issue, need other people input.

Also, attach a repository for reproduce this issue
https://github.com/kayac-chang/remix-abort-test/tree/main
https://gitpod.io/start/#kayacchang-remixabortte-0ohsnxppt54

I'm pretty sure Remix accepts PRs with a failing test case as a bug report, so if you're able to reproduce it in a test case and open a PR, that might bring this issue some attention.

Thank you for the suggestion, I will do that as soon as possible.

@firetripod
Copy link

i found this issue at fetch writeToStream(request_, request);
https://github.com/remix-run/web-std-io/blob/main/packages/fetch/src/fetch.js#L317C3-L317C16
bodey.js StreamIterableIterator.getReader not rleaseLock or cancel
who can fix it ??

@firetripod
Copy link

when the fetch request times out, abort will be triggered and an error will be throw

@brophdawg11 brophdawg11 added this to v2 Aug 3, 2023
@brophdawg11 brophdawg11 moved this to Backlog in v2 Aug 3, 2023
@paambaati
Copy link

@kayac-chang is spot on.

I think this issue can't be solve by clone the request, it is when request aborted will throw an error which even can't be catch from user side, so the server crash by this unhandled error.

This is a bit of a dealbreaker, as a server timeout (or a 500 error) is currently not catch-able at all.

@jacob-ebey jacob-ebey moved this from Backlog to In progress in v2 Aug 23, 2023
@brophdawg11 brophdawg11 moved this from In progress to Backlog in v2 Aug 31, 2023
@brophdawg11 brophdawg11 removed this from v2 Sep 5, 2023
@justinhandley
Copy link

I see this was assigned and then unassigned - any progress on this?

@cjoecker
Copy link

I see the Issue comes from people using Remix. In my case, it was breaking because I was reading the body more than once. Here you can read more about it.

@kayac-chang
Copy link
Contributor

kayac-chang commented Oct 24, 2023

The Remix team has been working on that; they want to remove the polyfill on node> = 18.
but the readable stream is still experimental so it may take some time.
remix-run/web-std-io#42

Thank the team for the fantastic work here! 🙏🙏🙏

@zinosama
Copy link

zinosama commented Apr 10, 2024

+1. Calling an endpoint that returns in 2s with a 1s timeout triggers the un-catchable error and crushes the process:

try { 
 const resp = await fetch("https://run.mocky.io/v3/b2cca8a5-81b6-4293-b957-774c399c416b?mocky-delay=2000ms", 
  {
    method: "POST",
    signal: AbortSignal.timeout(1000),
  });
} catch (error: any) {
  console.error(error)
}

output:

return promiseRejectedWith(new TypeError('Cannot cancel a stream that already has a reader'));                                           ^

TypeError: Cannot cancel a stream that already has a reader
    at ReadableStream2.ReadableStream2.cancel (.../node_modules/.pnpm/[email protected]/node_modules/web-streams-polyfill/dist/ponyfill.js:3587:44)
    at abort (.../node_modules/.pnpm/@[email protected]/node_modules/@remix-run/web-fetch/src/fetch.js:76:18)
    at EventTarget.abortAndFinalize (.../node_modules/.pnpm/@[email protected]/node_modules/@remix-run/web-fetch/src/fetch.js:92:4)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:762:20)
    at EventTarget.dispatchEvent (node:internal/event_target:704:26)
    at abortSignal (node:internal/abort_controller:365:10)
    at Timeout._onTimeout (node:internal/abort_controller:125:7)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)


Node.js v20.3.0
 ELIFECYCLE  Command failed with exit code 1.

@aldoreyes
Copy link

We also started encountering this error, something we see is that it only happens for POST | PUT requests, although this may very well be more related to requests sending a body in the request.

Here is our workaround

import { default as fetchNode } from 'node-fetch'

async function customFetch<T>(
	input: URL | RequestInfo,
	init?: RequestInit,
): Promise<ApiResponse<T>> {

        const fetchToUse =
		init?.method === 'POST' || init?.method === 'PUT' ? fetchNode : fetch

	const response = await fetchToUse(input, {
		...init,
                signal: AbortSignal.timeout(Number(process.env.FETCH_TIMEOUT ?? 25000)),
         });
         ...
}

@jacob-ebey
Copy link
Member

Please opt into "nativeFetch" on your installGlobals() call.

@simply-arjen
Copy link

simply-arjen commented Nov 15, 2024

Please opt into "nativeFetch" on your installGlobals() call.

We're running into this error as well, how does one opt into "nativeFetch" on the installGlobals() call? It does not accept any parameters. 🤔

Edit: I see we need to update to the latest version of remix then you can do:

installGlobals({nativeFetch: true})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:unverified feat:fetch Issues related to @remix-run/web-fetch
Projects
None yet
Development

No branches or pull requests