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

[BUG] Cancelling a response body stream returned from handleRequest results in an uncaught exception #2436

Open
lucacasonato opened this issue Jan 17, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@lucacasonato
Copy link

Describe the bug

When a platform plugin calls handleRequest, Hydrogen returns a Response object. This response object has a readable stream property (resp.body). If this stream is cancelled, for example because the user has preemptively hung up the connection, Hydrogen will crash the process with an uncaught promise rejection error.

    const resp = await handleRequest(request, {
      indexTemplate,
      context: { waitUntil, ...context },
      cache,
    });
    const reader = resp!.body!.getReader();
    reader.cancel("cancelled");

    // an uncaught rejection is raised here

To Reproduce

I was going to write you an integration test to demonstrate the issue, but I realized you don't have integration tests for handleRequest. As such, I don't know how to best write a reproduction for you here.

If there is a file with handleRequest tests that I can modify, please point me at it.

Expected behaviour

I expect the process to not crash with an uncaught rejection.

Root cause

I have not thoroughly investigated, but I am about 90% confident you are incorrectly using a WritableStream somewhere in Hydrogen (this is where this class of errors most often comes from). What is likely happening is that you are creating a TransformStream, and picking off the readable and writable end of this stream, passing the readable end as the new Response body. You are then calling writable.getWriter(), and are then calling writer.write() without handling the promise returned from writer.write(). You need to handle promise rejections here! Because you are not doing this, there is now a floating promise that upon rejection takes down the entire process. A cursory grep of the repo suggests that the culprits may be one or all of these writes:

I suggest you either await these writes, or add catch handlers to their returned promises.

@lucacasonato lucacasonato added the bug Something isn't working label Jan 17, 2023
@lucacasonato
Copy link
Author

You can observe this behaviour by running this code:

const { readable, writable } = new TransformStream();

const reader = readable.getReader();
reader.cancel("cancelled");

const writer = writable.getWriter();
writer.write("a");

vs this code:

const { readable, writable } = new TransformStream();

const reader = readable.getReader();
reader.cancel("cancelled");

const writer = writable.getWriter();
writer.write("a").catch(() => {});

@blittle
Copy link
Contributor

blittle commented Jan 17, 2023

Hi @lucacasonato, thank you for reporting this. We are in the process of migrating Hydrogen to use Remix under the hood. This problem should go away with the new release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants