-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Possible memory leak when using server side events #11601
Comments
What if you remove that |
It's do nothing actually, just for show purposes. When subscribing to the route, start pushing values after 1 sec, nothing more. |
then I don't think that that code is enough to reproduce your issue, right? Please share the full code along with the steps to reproduce. why reproductions are required |
example nest application you can use to reproduce this behavior. after running the application, use Postman or any other similar application and enter |
Seems like some sort of issue with usd adding Stack Trace
|
As I continue debugging, find out this comment and think it's related to this. I don't know why it does not appear when you use |
cc @soyuka would you like to take a look? |
I have the same issue: |
@ggagosh could you please provide an example with |
I have the same issue, has anyone found some workaround/solution to this problem? In my case, I can reproduce the issue by sending HTTP requests and immediately cancelling them, after a couple of cancelled requests the warning pops up. |
same here |
I figured out a solution: Add a listener for disconnect event, which stops the streaming. @Sse('stream`)
sse(@Req() req: Request) {
const stream = /* start your stream */
req.on('close', ()=>{ stream.destroy() })
} or something like that. |
I've submitted a PR requesting some changes to solve this issue. If you could validate it, I would appreciate it. #12188 |
Hi, sorry didn't see that, but it doesn't surprise me that there's a leak as by definition we may never end the stream... You should consider using mercure.rocks for something more bullet proof. @lponciolo does the patch really concern SSE ? @lxmfly123 solution above is quite correct if you need to end the stream. We can close this. |
@soyuka In my case, the problem occurs when, having several gateways that use @SubscribeMessage, a client connects to the websocket, that bindMessageHandlers function also runs several times generating listeners for the same disconnect event, then the warning appears. The issue at hand appears to be somewhat different from my own situation; I apologize for any confusion. You might find this issue to be more relevant: #7249 |
@lxmfly123 I (like in the repro) use |
I call |
In my case, I can reproduce this by aborting the client connection. Basically, start a SSE stream on the browser and then hit F5 to reload the webpage. This makes the browser disconnect and then the server will show this error. |
I was able to fix my issue by completing the observable when the request socket is closed.
You can use an AbortController and signal it to tell your other code to stop emitting events. |
To solve this issue we should just document that developers should end the stream on client socket close. Can someone contribute that at https://github.com/nestjs/docs.nestjs.com/blob/bf400a0c615185a72fd58998526fdfaec19ac5d1/content/techniques/server-sent-events.md?plain=1#L3 ? Thanks! |
@soyuka I quite confused with these warnings (because it also throws to nestjs queue), and quite misleading. The problem is happening when the redis service not on and not working. Will there any notes for this or at least it doesn't cause confusion with the developer with throwing error that redis connection is broken. |
Note sure how this related to redis |
With redis it thow same error message and warning when the connection isn't initiated and when using the consumers I think it's OOT with this, but it produce the same warnings when the redis server is dead, case to missleading, it should throw error instead. |
I'm still having this issue with Redis-based queues |
@joaopcm Can you share a minimum reproduction? I'd like to investigate it. |
Is there an existing issue for this?
Current behavior
I use Server-Sent Events for one of my routes and then push db changes for subscribed users. The functionality works fine, but as soon as the event count is 10k for example, the error appears:
(node:43243) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 101 drain listeners added to [SseStream]. Use emitter.setMaxListeners() to increase limit
Minimum reproduction code
https://gist.github.com/ggagosh/0850fd0100f17e2bc040095a917c0342
Steps to reproduce
No response
Expected behavior
I suppose should work without any warning
Package
@nestjs/common
@nestjs/core
@nestjs/microservices
@nestjs/platform-express
@nestjs/platform-fastify
@nestjs/platform-socket.io
@nestjs/platform-ws
@nestjs/testing
@nestjs/websockets
Other package
No response
NestJS version
^9.0.0
Packages versions
Node.js version
18.16.0
In which operating systems have you tested?
Other
No response
The text was updated successfully, but these errors were encountered: