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

Uncatchable Exception when response stream is closed #23058

Closed
irbull opened this issue Mar 24, 2024 · 5 comments
Closed

Uncatchable Exception when response stream is closed #23058

irbull opened this issue Mar 24, 2024 · 5 comments

Comments

@irbull
Copy link
Contributor

irbull commented Mar 24, 2024

Version: Deno 1.41.3

In a request handler, I created a stream to write data back to the response. If the client closes the connection, the next time the writer is access, an exception is thrown and server crashes. Unfortunately I can't find any way to catch the exception as it's likely happening under the covers. Even if I add a close listener to the writer, it will throw.

const handler = (_request: Request): Response => {
  const stream = new TextEncoderStream();
  const writer = stream.writable.getWriter();
  writer.closed.then(() => {
    console.log("stream closed");
  });

  setInterval(() => {
    writer.write("data\n");
  }, 1000);

  return new Response(stream.readable, {
    status: 200,
  });
};

console.log(`HTTP server running. Access it at: http://localhost:8080/`);
Deno.serve({ port: 8000 }, handler);

If I then try to reach this Endpoint via something like curl:

$ curl http://localhost:8000
data
data
data

It will return the streamed data. If I kill the curl process (ctr+c), the stream is closed and the server dies with the following:

error: Uncaught (in promise) "resource closed"

Any thoughts on how to catch this exception. It happens on a Mac, I haven't tested it on other platforms.

@irbull
Copy link
Contributor Author

irbull commented Mar 25, 2024

I looked at this again. The exception is coming from the doing things with the writer. I think the locking needs to be tighter. The following handles the exception.

const handler = (_request: Request): Response => {
  const stream = new TextEncoderStream();

  setInterval(() => {
    const writer = stream.writable.getWriter();
    writer.write("data\n").catch((e) => {
      console.log("Error: ", e);
    }).finally(() => {
      writer.releaseLock();
    });
  }, 1000);

  return new Response(stream.readable, {
    status: 200,
  });
};

console.log(`HTTP server running. Access it at: http://localhost:8080/`);
Deno.serve({ port: 8000 }, handler);

This solution doesn't stop the interval, but I'll leave that as an exercise for the reader :).

@irbull irbull closed this as completed Mar 25, 2024
@danopia
Copy link
Contributor

danopia commented May 2, 2024

I'm seeing this uncatchable exception in other codebases. Am I correct in understanding that the program will crash if a client walks away while there is a writer locked onto the response stream?

@mmastrac
Copy link
Contributor

mmastrac commented May 2, 2024

If you're using a TransformStream as shown here, you definitely need to catch the write exceptions as those are how the close signal is propagated back through the chain.

We should definitely improve this error message to help point to the source of the error, however.

Note that locking and unlocking the stream is a bit unnecessary -- you should be able to just get the writer once and keep writing to it.

@irbull
Copy link
Contributor Author

irbull commented May 2, 2024

IIRC I didn't realize that the write was returning a promise, and at first I just wrapped it in a try / catch without an await. This seems to be a source of frustration for more than a few people, especially if you don't actually need the result of the call, then it's very easy to forget the await.

@mmastrac is right, you don't need to lock and unlock continually, although if others need the writer than you may need to do this. This isn't best design, but the point was that it appeared to be uncatchable, but in the end it wasn't (which is why I closed this). I have seen other cases of uncatchable exceptions though (mostly around web sockets) but that was 2+ years ago and I think they have been resolved now.

@danopia
Copy link
Contributor

danopia commented May 11, 2024

Ok, I was able to track down and fix this uncaught exception within my own codebase (/x/observability). I confused it as uncatchable since it seems to be a string instead of an Error and thus has no stacktrace. Since my code is to instrument HTTP serving, I can now attach this exception to the request span and keep the overall server alive. Thank you two for the context!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants