nest icon indicating copy to clipboard operation
nest copied to clipboard

Possible memory leak when using server side events

Open ggagosh opened this issue 1 year ago • 24 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

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

  • [ ] I don't know. Or some 3rd-party package
  • [X] @nestjs/common
  • [ ] @nestjs/core
  • [ ] @nestjs/microservices
  • [ ] @nestjs/platform-express
  • [ ] @nestjs/platform-fastify
  • [ ] @nestjs/platform-socket.io
  • [ ] @nestjs/platform-ws
  • [ ] @nestjs/testing
  • [ ] @nestjs/websockets
  • [ ] Other (see below)

Other package

No response

NestJS version

^9.0.0

Packages versions

"@nestjs/common": "^9.0.0",
"@nestjs/config": "2.3.1",
"@nestjs/core": "^9.0.0",
"@nestjs/event-emitter": "^1.4.1",
"@nestjs/graphql": "^11.0.5",
"@nestjs/jwt": "10.0.3",
"@nestjs/mongoose": "^9.2.2",
"@nestjs/passport": "9.0.3",
"@nestjs/platform-express": "^9.0.0",
"@nestjs/swagger": "6.3.0",
"@nestjs/terminus": "^9.2.2",

Node.js version

18.16.0

In which operating systems have you tested?

  • [X] macOS
  • [ ] Windows
  • [ ] Linux

Other

No response

ggagosh avatar May 05 '23 17:05 ggagosh

What if you remove that setTimeout call?

micalevisk avatar May 05 '23 17:05 micalevisk

What if you remove that setTimeout call?

It's do nothing actually, just for show purposes. When subscribing to the route, start pushing values after 1 sec, nothing more.

ggagosh avatar May 05 '23 17:05 ggagosh

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

micalevisk avatar May 05 '23 17:05 micalevisk

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 localhost:3000/sse, then check the console where the warning appears.

ggagosh avatar May 05 '23 18:05 ggagosh

Seems like some sort of issue with usd adding this.once('drain') here. Interestingly it doesn't happen when using rxjs directly via interval

Stack Trace
(node:1588817) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [SseStream]. Use emitter.setMaxListeners() to increase limit
    at _addListener (node:events:587:17)
    at SseStream.addListener (node:events:605:10)
    at SseStream.Readable.on (node:internal/streams/readable:875:35)
    at SseStream.once (node:events:649:8)
    at SseStream.writeMessage (/home/jay/Documents/code/help/sse/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/router/sse-stream.js:70:18)
    at /home/jay/Documents/code/help/sse/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/router/router-response-controller.js:65:80
    at new Promise (<anonymous>)
    at /home/jay/Documents/code/help/sse/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected][email protected][email protected]/node_modules/@nestjs/core/router/router-response-controller.js:65:50
    at /home/jay/Documents/code/help/sse/node_modules/.pnpm/[email protected]/node_modules/rxjs/src/internal/operators/debounce.ts:101:21

jmcdo29 avatar May 05 '23 20:05 jmcdo29

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 interval directly, maybe because of debounse?

ggagosh avatar May 06 '23 19:05 ggagosh

cc @soyuka would you like to take a look?

kamilmysliwiec avatar May 17 '23 06:05 kamilmysliwiec

I have the same issue: (node:167175) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [SseStream]. Use emitter.setMaxListeners() to increase limit

azhaochen avatar May 29 '23 14:05 azhaochen

@ggagosh could you please provide an example with interval?

Furmanov19 avatar May 31 '23 09:05 Furmanov19

@ggagosh could you please provide an example with interval?

Hey, this is the example branch

I take the interval example from NestJS documentation, I just change the interval time to 0, because first I think the warning was caused by the amount of data I try to return.

ggagosh avatar May 31 '23 09:05 ggagosh

I have the same issue: (node:167175) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [SseStream]. Use emitter.setMaxListeners() to increase limit

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.

MatheusWoeffel avatar Jun 14 '23 16:06 MatheusWoeffel

same here

dgastudio avatar Jun 26 '23 06:06 dgastudio

I have the same issue: (node:167175) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [SseStream]. Use emitter.setMaxListeners() to increase limit

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.

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.

lxmfly123 avatar Jul 09 '23 04:07 lxmfly123

I've submitted a PR requesting some changes to solve this issue. If you could validate it, I would appreciate it. https://github.com/nestjs/nest/pull/12188

lponciolo avatar Aug 03 '23 12:08 lponciolo

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 avatar Aug 03 '23 12:08 soyuka

@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: https://github.com/nestjs/nest/issues/7249

lponciolo avatar Aug 04 '23 01:08 lponciolo

@lxmfly123 I (like in the repro) use Subject from rxjs, which doesn't seem to have anything resembling a .destroy() message. Any idea how to update it for the rxjs case?

chriskuech avatar Dec 05 '23 18:12 chriskuech

@lxmfly123 I (like in the repro) use Subject from rxjs, which doesn't seem to have anything resembling a .destroy() message. Any idea how to update it for the rxjs case?

I call subject.complete()

Totalus avatar Dec 05 '23 18:12 Totalus

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.

codemile avatar Dec 06 '23 14:12 codemile

I was able to fix my issue by completing the observable when the request socket is closed.

import {Request} from 'express';


@Sse()
see(@Req() req: Request): Observable<any> {
   return new Observable((subscriber) => {
      req.socket.on('close', () => {
          subscriber.complete();
      });
     // do your stream work here
   });
}

FYI: req.on('close') didn't work for me. Had to use req.socket.on('close') instead.

You can use an AbortController and signal it to tell your other code to stop emitting events.

codemile avatar Dec 06 '23 15:12 codemile

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 avatar Dec 07 '23 11:12 soyuka

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

benyaminl avatar May 11 '24 05:05 benyaminl

Note sure how this related to redis

soyuka avatar May 11 '24 10:05 soyuka

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 https://docs.nestjs.com/techniques/queues#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.

benyaminl avatar May 17 '24 12:05 benyaminl