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

Astro request.signal abort event not called for SSE #12867

Open
1 task done
mihaiandrei97 opened this issue Dec 31, 2024 · 4 comments
Open
1 task done

Astro request.signal abort event not called for SSE #12867

mihaiandrei97 opened this issue Dec 31, 2024 · 4 comments
Labels
- P2: nice to have Not breaking anything but nice to have (priority) feat: dev Related to `astro dev` CLI command (scope) help wanted Please help with this issue! pkg: node Related to Node adapter (scope)

Comments

@mihaiandrei97
Copy link

mihaiandrei97 commented Dec 31, 2024

Astro Info

Astro                    v5.1.1
Node                     v20.9.0
System                   macOS (arm64)
Package Manager          npm
Output                   server
Adapter                  @astrojs/node
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

While trying to implement SSE with node adapter in Astro, it seems the abort event is not triggered on request.signal when the connection is closed.

export const clients = new Set() as Set<(message: string) => void>;

export const GET: APIRoute = ({ request }) => {
  const responseHeaders = {
    "Content-Type": "text/event-stream",
    "Cache-Control": "no-cache",
    Connection: "keep-alive",
  };

  const stream = new ReadableStream({
    start(controller) {
      const encoder = new TextEncoder();

      // Function to send messages to this client
      const send = (message: string) => {
        controller.enqueue(encoder.encode(`data: ${message}\n\n`));
      };

      // Add the send function to the set of clients
      clients.add(send);

      // Send an initial message
      send("Connected to SSE");

      request.signal.addEventListener("abort", () => {
        console.log("Client disconnected ");
        clients.delete(send);
        controller.close();
      });

    },
  });

  return new Response(stream, { headers: responseHeaders });
};

What's the expected result?

The following callback should be called when the client disconnects.

request.signal.addEventListener("abort", () => {
  console.log("Client disconnected ");
});

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-yi4swteg?file=src%2Fpages%2Fsse.ts

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Dec 31, 2024
@ematipico ematipico added the needs response Issue needs response from OP label Jan 1, 2025
@ematipico
Copy link
Member

Can tell us how to reproduce the issue and how this signal/SSE is supposed to work?

@mihaiandrei97
Copy link
Author

In the reproduction here: https://stackblitz.com/edit/github-yi4swteg?file=src%2Fpages%2Fsse.ts
You can go to the /sse page. There you will see data: Connected to SSE. Now, anywhere in your app you can use the clients to send events, and the user will see those realtime in the /sse endpoint. That part works and it is not part of the issue.

However, when the user closes the tab, the abort event should be sent and be caught on request.signal. It seems that one isn't triggered.

@ascorbic
Copy link
Contributor

ascorbic commented Jan 7, 2025

This issue here is that Astro doesn't support AbortSignals at all. Astro creates the web Request object, mainly from Node IncomingMessage objects, which don't support abort signals natively so the generated requests have no signal passed to them. Support could in theory be added to createRequest, which is the function that converts a Node IncomingMessage into a web Request, but right now it's not implemented. It would involve creating an AbortController inside NodeApp.createRequest and passing the signal from that to the Request constructor. It would need to listen to the IncomingMessage close() event, and abort the controller from there. It would also need implementing in the other createRequest in core/request, and probably in adapters too otherwise it would be lost before being passed to the user code. So basically, this is a feature that could be added, but isn't really a bug.

@ascorbic ascorbic added help wanted Please help with this issue! - P2: nice to have Not breaking anything but nice to have (priority) pkg: node Related to Node adapter (scope) feat: dev Related to `astro dev` CLI command (scope) and removed needs response Issue needs response from OP needs triage Issue needs to be triaged labels Jan 7, 2025
@mihaiandrei97
Copy link
Author

Thank you! You're right. Using cancel works. Posting an example if anyone has the same use-case:

export const GET: APIRoute = ({ request }) => {
  const responseHeaders = {
    "Content-Type": "text/event-stream",
    "Cache-Control": "no-cache",
    Connection: "keep-alive",
  };

  const stream = new ReadableStream({
    start(controller) {
      const encoder = new TextEncoder();

      // Function to send messages to this client
      const send = (message: string) => {
        controller.enqueue(encoder.encode(`data: ${message}\n\n`));
      };

      // Send an initial message
      send("Connected to SSE");

    },
     cancel() {
      // Cleanup logic if needed
      console.log("Client disconnected")
    },
  });

  return new Response(stream, { headers: responseHeaders });
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: nice to have Not breaking anything but nice to have (priority) feat: dev Related to `astro dev` CLI command (scope) help wanted Please help with this issue! pkg: node Related to Node adapter (scope)
Projects
None yet
Development

No branches or pull requests

3 participants