Native HTTP: A streaming body/ServerSentEvents fails

kitsonk commented 3 years ago

I would be able to expect to be able to utilise SSE with the native HTTP, and I can start a request, by setting the appropriate headers and return a ReableStream as a body, pushing "chunks" into the controller that represent the events.

But when the client attempts to navigate away or refresh, an unrecoverable/uncatchable error occurs:

error: Uncaught (in promise) Http: connection closed before message completed
    at unwrapOpResult (deno:core/core.js:100:13)
    at async Object.respondWith (deno:runtime/js/40_http.js:153:11)

What I would have expected is that if the Response init was a readable stream (as it is in this case) the ReadableStream would be cancelled with the reason, which I would handle in the server code "gracefully".

kitsonk commented 3 years ago

@ry it was more than 5 lines, but here is an example:

const INDEX_HTML = `<!DOCTYPE html>
<html>
  <head></head>
  <body>
    <h1>Hello world!</h1>
    <ul id="events"></ul>
    <script>
      const sse = new EventSource("/sse");
      const ul = document.getElementById("events");
      sse.onmessage = (evt) => {
        const li = document.createElement("li");
        li.textContent = \`message: \${evt.data}\`;
        ul.appendChild(li);
      };
    </script>
  </body>
</html>
`;

const encoder = new TextEncoder();

function handleRequest({ request, respondWith }: Deno.RequestEvent) {
  let respond: (response: Response) => void;
  const p = new Promise<Response>((r) => respond = r);
  const respondWithPromise = respondWith(p);
  if (request.url === "/sse") {
    let id = 0;
    let interval = 0;
    const body = new ReadableStream({
      start(controller) {
        console.log("start");
        interval = setInterval(() => {
          controller.enqueue(
            encoder.encode(
              `event: message\nid: ${++id}\ndata: { "hello": "deno" }\n\n`,
            ),
          );
        }, 1000);
      },
      cancel() {
        console.log("cancelled");
        clearInterval(interval);
      },
    });
    respond!(
      new Response(body, {
        headers: {
          "Connection": "Keep-Alive",
          "Content-Type": "text/event-stream",
          "Cache-Control": "no-cache",
          "Keep-Alive": `timeout=${Number.MAX_SAFE_INTEGER}`,
        },
      }),
    );
  } else {
    respond!(
      new Response(INDEX_HTML, {
        headers: {
          "content-type": "text/html",
        },
      }),
    );
  }
  return respondWithPromise;
}

for await (const conn of Deno.listen({ port: 8000 })) {
  const httpConn = Deno.serveHttp(conn);
  (async () => {
    for await (const requestEvent of httpConn) {
      await handleRequest(requestEvent);
    }
  })();
}

console.log("started");

Run with:

> deno run --allow-net --unstable https://gist.githubusercontent.com/kitsonk/057eb9e8c456835a7a6cce14bfcb6777/raw/c7ecd527d43fd495754cc99f71e691b475aef510/sseExample.ts

Navigate to http://localhost:8000/ in a browser and start seeing events pop onto the page, then close the tab, or hit reload, or anything to terminate the page and it should result in:

error: Uncaught (in promise) Http: connection closed before message completed
      await handleRequest(requestEvent);
      ^
    at unwrapOpResult (deno:core/core.js:100:13)
    at async respondWith (deno:runtime/js/40_http.js:153:11)

But I just realised with #10197 the error is now catchable, which means I can work around it. I would still expect the ReadableStream to be cancelled though, with this reason, instead of the respondWith() promise being rejected (or in addition to).

nayeemrmn commented 3 years ago

I would still expect the ReadableStream to be cancelled though, with this reason, instead of the respondWith() promise being rejected (or in addition to).

I think it should be in addition to. The consequences of op_http_response_write failing are 1) the response failed so respondWith() should reject and 2) the rest of the data is no longer needed so the passed body should be cancel()ed. I don't think (2) should replace (1) even if they are both receivers of the error. The first is the reason for the response failing and the second is the reason for the stream no longer being needed. The respondWith() call needs robust error handling around it either way, the initial op_http_response could probably also fail due to the client.

But I just realised with #10197 the error is now catchable

In my early testing (specifically with SSE in fact) it was always catchable, but for a moment before that I thought it wasn't because of a stack trace bug: https://discord.com/channels/684898665143206084/684911491035430919/829893036468338729. Maybe you ran into the same thing?

kitsonk commented 3 years ago

Maybe you ran into the same thing?

🤷 possibly... but then #10197 magically fixed the stack trace too...

nayeemrmn commented 3 years ago

The stack traces were fixed by #10080, I can confirm that respondWith() errors are catchable before #10197. (Sorry for the tangent 🙏)

juzi5201314 commented 3 years ago

emmm, I also encountered this error when using Native HTTP. The relevant code is the same as the sample code of Release Notes. When using wrk2 to test, when wrk2 is finished. Deno was built on the master branch just pulled.

nayeemrmn commented 3 years ago

@juzi5201314 The error is expected, here is the same example with error handling and logging:

const body = new TextEncoder().encode("Hello World");
for await (const conn of Deno.listen({ port: 4500 })) {
  (async () => {
    for await (const { respondWith } of Deno.serveHttp(conn)) {
      try {
        await respondWith(new Response(body));
      } catch (error) {
        console.error("%cResponse failed%c:", "color: #909000", "", error);
      }
    }
  })();
}

(I still have no idea if using .catch() here would be faster than awaiting.)

denoland / deno

Native HTTP: A streaming body/ServerSentEvents fails #10193