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

@astrojs/node-6.1.0 seems to have broken SSE functionality #9239

Closed
1 task
0xk1f0 opened this issue Nov 29, 2023 · 11 comments
Closed
1 task

@astrojs/node-6.1.0 seems to have broken SSE functionality #9239

0xk1f0 opened this issue Nov 29, 2023 · 11 comments
Labels
needs response Issue needs response from OP needs triage Issue needs to be triaged

Comments

@0xk1f0
Copy link

0xk1f0 commented Nov 29, 2023

Astro Info

> astro info

Astro                    v3.6.3
Node                     v21.2.0
System                   Linux (x64)
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

I noticed that @astrojs/node-6.1.0 seems to have broken SSE in my project. I used the reference implementation in this astro-examples Repository. It is a bit out of date but can be used to reproduce by bumping the dependencies

"dependencies": {
    "astro": "3.6.3",
    "@astrojs/node": "6.1.0"
}

With this configuration, SSE works when first connecting to the site (running via npm run preview). The issue arises when you reload the site, as the counter stops and astro crashes with:

12:02:41 AM [@astrojs/node] Server listening on http://127.0.0.1:4321
events.js> start_counting() started:false
12:02:44 AM [astro] Lower case endpoint names are deprecated and will not be supported in Astro 4.0. Rename the endpoint get to GET.
stream.js> get()
events.js> counter = 1
stream.js> Emitter 'count' = 1
events.js> counter = 2
stream.js> Emitter 'count' = 2
events.js> counter = 3
stream.js> Emitter 'count' = 3
stream.js> cancel()
12:02:50 AM [astro] Lower case endpoint names are deprecated and will not be supported in Astro 4.0. Rename the endpoint get to GET. (x2)
stream.js> get()
events.js> counter = 4
stream.js> Emitter 'count' = 4
node:internal/webstreams/readablestream:1074
      throw new ERR_INVALID_STATE.TypeError('Controller is already closed');
      ^

TypeError [ERR_INVALID_STATE]: Invalid state: Controller is already closed
    at ReadableStreamDefaultController.enqueue (node:internal/webstreams/readablestream:1074:13)
    at EventEmitter.events_listener (file:///home/k1f0/astro-examples/03_sse-counter/dist/server/chunks/pages/stream_fc3529f5.mjs:12:28)
    at EventEmitter.emit (node:events:531:35)
    at Timeout._onTimeout (file:///home/k1f0/astro-examples/03_sse-counter/dist/server/chunks/pages/events_e3ddce38.mjs:17:17)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7) {
  code: 'ERR_INVALID_STATE'
}

Node.js v21.2.0

What's the expected result?

As noted above with the configuration of

"dependencies": {
    "astro": "3.6.3",
    "@astrojs/node": "6.0.0"
}

SSE works as expected, even after reloading the site and counts up normally:

12:00:01 AM [@astrojs/node] Server listening on http://127.0.0.1:4321
events.js> start_counting() started:false
12:00:04 AM [astro] Lower case endpoint names are deprecated and will not be supported in Astro 4.0. Rename the endpoint get to GET.
stream.js> get()
events.js> counter = 1
stream.js> Emitter 'count' = 1
events.js> counter = 2
stream.js> Emitter 'count' = 2
events.js> counter = 3
stream.js> Emitter 'count' = 3
12:00:11 AM [astro] Lower case endpoint names are deprecated and will not be supported in Astro 4.0. Rename the endpoint get to GET. (x2)
stream.js> get()
events.js> counter = 4
stream.js> Emitter 'count' = 4
stream.js> Emitter 'count' = 4
events.js> counter = 5
stream.js> Emitter 'count' = 5
stream.js> Emitter 'count' = 5
...

Link to Minimal Reproducible Example

https://github.com/0xk1f0/astro-examples/tree/main/03_sse-counter

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 Nov 29, 2023
@bluwy
Copy link
Member

bluwy commented Nov 30, 2023

The only change we have in 6.1.0 is #9125 and I'm not sure how that's causing the issue.

I think this is expected though. When you reload (presumably) http://localhost:4321/api/stream, you disconnect the stream, which causes the ReadableStream to cancel here: https://github.com/0xk1f0/astro-examples/blob/97828b7e404b01a0fca52330cc13fdaf956865b3/03_sse-counter/src/pages/api/stream.js#L17-L19

Which means you can no longer enqueue more data or else it'll error. I don't think @astrojs/node had changed that and is how streamed response work normally? One fix is to stop enqueue-ing data when the ReadableStream has been cancelled.

@bluwy bluwy added the needs response Issue needs response from OP label Nov 30, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Nov 30, 2023

Seconding bluwy, I think the current behavior is expected. The alternative is that your ReadableStreams continue to produce chunks long after the visitor has disconnected, possibly as long as the server stays online.

@0xk1f0
Copy link
Author

0xk1f0 commented Nov 30, 2023

Ok that makes sense, thanks for looking into this!

I'm kinda new to SSE so could anyone point me in the right direction on how to implement this correctly? As cancelling the ReadableStream seems like the right choice when the stream gets disconnected, how can I make it survive a page reload? Would I need to reinitialize this in the client side JS or am i missing some logic in the endpoint?

@lilnasy
Copy link
Contributor

lilnasy commented Nov 30, 2023

how can I make it survive a page reload

@0xk1f0 Looking at @wassfila's example that you are following, the server state (let counter = 0) will survive and keep increasing. It lives independently of requests.

Would I need to reinitialize this in the client side JS

Not manually, when a visitor comes back they will start receiving events from that point on, same as they would have on first visit.

@0xk1f0
Copy link
Author

0xk1f0 commented Nov 30, 2023

the server state (let counter = 0) will survive and keep increasing

That's fair, but I meant survive in terms of "how do i not make it crash" ? :D

@lilnasy
Copy link
Contributor

lilnasy commented Nov 30, 2023

You could unsubscribe the callback from the EventEmitter.

I am not familiar with Node's EventEmitter, but if it was an EventTarget, you could create an AbortController for each request, pass its .signal to the addEventListener call, and call abort() on it when the stream cancels.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 30, 2023

Closing as the current behavior is correct.

@lilnasy lilnasy closed this as not planned Won't fix, can't repro, duplicate, stale Nov 30, 2023
@0xk1f0
Copy link
Author

0xk1f0 commented Nov 30, 2023

For anyone that might come across this, I managed to make it work properly.

I published the modified example here.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 30, 2023

I see you are using Emitter.removeAllListeners. What would happen if 3 users are connected at the same time and then only one disconnects?

@0xk1f0
Copy link
Author

0xk1f0 commented Nov 30, 2023

What would happen if 3 users are connected at the same time and then only one disconnects?

That's a good point! Adjusted my example, I think it should be more appropriate now.

@wassfila
Copy link

wassfila commented Dec 2, 2023

thanks @0xk1f0 I updated my sample with your cancel fix and mentioned your repo https://github.com/MicroWebStacks/astro-examples#03_sse-counter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs response Issue needs response from OP needs triage Issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

4 participants