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

Native HTTP: A streaming body/ServerSentEvents fails #10193

Closed
kitsonk opened this issue Apr 15, 2021 · 6 comments · Fixed by #10225
Closed

Native HTTP: A streaming body/ServerSentEvents fails #10193

kitsonk opened this issue Apr 15, 2021 · 6 comments · Fixed by #10225
Labels
bug Something isn't working correctly runtime Relates to code in the runtime crate

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Apr 15, 2021

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 kitsonk added bug Something isn't working correctly runtime Relates to code in the runtime crate labels Apr 15, 2021
dhardtke added a commit to dhardtke/prandium that referenced this issue Apr 15, 2021
@kitsonk
Copy link
Contributor Author

kitsonk commented Apr 15, 2021

@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
Copy link
Collaborator

nayeemrmn commented Apr 16, 2021

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
Copy link
Contributor Author

kitsonk commented Apr 16, 2021

Maybe you ran into the same thing?

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

@nayeemrmn

This comment has been minimized.

@juzi5201314
Copy link
Contributor

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
Copy link
Collaborator

@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.)

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

Successfully merging a pull request may close this issue.

3 participants