nest icon indicating copy to clipboard operation
nest copied to clipboard

Multiple gateways with socket.io get (disconnect listeners added to [Socket])

Open MedianAura opened this issue 3 years ago • 16 comments

Bug Report

Current behavior

image

Input Code

https://github.com/MedianAura/gateway-test

Just use : npm run start:dev

Go to : http://127.0.0.1:3000/

Expected behavior

Not to get this warning or at least understand what can cause it and what is the risk

Possible Solution

N/A

Environment


Nest version: 7.6.0

 
For Tooling issues:
- Node version: 14.17.0
- Platform:  Windows 10

Others:
yarn

MedianAura avatar Jun 09 '21 21:06 MedianAura

Your socketio version ol for the public/index.html is v4 which is incompatible with socketio v2 (what nest uses for the server). Please downgrade your client and check again

jmcdo29 avatar Jun 09 '21 22:06 jmcdo29

Downgraded to 2.4.0 and still the same issue just a tiny bit less spammy

MedianAura avatar Jun 09 '21 23:06 MedianAura

Cool, wanted to rule that out. It looks like for every gateway class (a class decorated with @WebSocketGateway()), two disconnect handlers get bound to the client instance. One is a function named handler and one is an anonymous function. I'm trying to determine where they are bound from, but you can see in this log how this builds up quickly (I changed all of the @SubscribeMessage()s to have specific letters to correspond with the gateway to make debugging easier)

[]
[
  { message: 'a', callback: [Function: bound ] AsyncFunction },
  { message: 'a2', callback: [Function: bound ] AsyncFunction }
]
[ [Function: handler], [Function (anonymous)] ]
[ { message: 'b', callback: [Function: bound ] AsyncFunction } ]
[
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)]
]
[ { message: 'c', callback: [Function: bound ] AsyncFunction } ]
[
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)]
]
[ { message: 'd', callback: [Function: bound ] AsyncFunction } ]
[
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)]
]
[ { message: 'e', callback: [Function: bound ] AsyncFunction } ]
[
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)],
  [Function: handler],
  [Function (anonymous)]
]
[ { message: 'f', callback: [Function: bound ] AsyncFunction } ]
(node:26549) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 disconnect listeners added to [Socket]. Use emitter.setMaxListeners() to increase limit

This is from adding a console.log in the io-adapter.js file in the node_modules. There should probably be some way to not bind new disconnect handlers, but I'd need more time with the code.

jmcdo29 avatar Jun 09 '21 23:06 jmcdo29

@jmcdo29 Please take a look at https://github.com/nestjs/nest/issues/6026#issuecomment-881339664, I believe it's related

genadis avatar Jul 31 '21 12:07 genadis

So is there any solution?

itspers avatar Jan 14 '22 00:01 itspers

I would also appreciate a solution for this. At this moment I am creating many different classes using the @WebSocketGateway() decorator and adding them to their respective modules which are then imported in the general app.module

Is this the right way of doing things or should I create a gateway module that imports the other modules and handle all the messages? I could do that but it seems that this would defeat the purpose of having the code organized by fields instead of functionality.

I saw the option of creating a custom adapter to raise the connection limit, but from what I see I would have to keep increasing the limit the more gateway classes I add.

Thanks in advance! =)

FredericoGauz avatar Apr 22 '22 18:04 FredericoGauz

I think this issue should be re-opened, as this is getting spammy really fast

RDeluxe avatar Jun 01 '22 14:06 RDeluxe

@FredericoGauz hello, just curious if you're succeed creating a gateways mediator? I gave it a much thought and it seems to be a good API dictionary, alas breaks separation of concerns. Thank you in advance!

alterfo avatar Aug 23 '22 21:08 alterfo

hello, I'm on a very important place in the project. I think the dto is not working properly can you help me?

osmankorogluu avatar Sep 27 '22 13:09 osmankorogluu

Hi Oleg,

Unfortunately I didn't, what I did was create a separate module that handles one Gateway and import the other modules, handling their requests.

It is far from ideal, but at least is working and it should not be very hard to refractor if we have a chance to do it if something changes in the future.

Let me know if I can be of any help. If you are taking generally about nestjs it is very interesting, but there is definitely a trade off between code quality and difficult to use. Sometimes nestjs makes things very easy, but other times I feel like it is a nightmare. Probably because don't understand it as much as I should.

Thanks

On Tue, Aug 23, 2022, 6:31 PM Oleg Sidorkin @.***> wrote:

@FredericoGauz https://github.com/FredericoGauz hello, just curious if you're succeed creating a gateways mediator? I gave it a much thought and it seems to be a good API dictionary, but breaks separation of concerns. Thank you in advance!

— Reply to this email directly, view it on GitHub https://github.com/nestjs/nest/issues/7249#issuecomment-1224907299, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEL2SSUQK2GENLA55MRJZT3V2U7JPANCNFSM46M3TJJA . You are receiving this because you were mentioned.Message ID: @.***>

FredericoGauz avatar Oct 11 '22 07:10 FredericoGauz

Is there a solution to this error?

Karman40 avatar Nov 03 '22 15:11 Karman40

I too faced this issue and have found the root cause of the problem. It is in package @nestjs/platform-socket.io

Here is what is happening for every event the client(browser) connects to backed through gateway the function bindMessageHandlers. The function internally creates a listener to the disconnect event of socket which is further used to stop processing events propagated by server to client. Hence if you have 20 such events being subscribed by client the total number of disconnect would be 20 +2. 20 disconnect listeners for each event subscribed and 2 internal system event handling in which 2nd system based event subscription is conditional but still worth mentioning it.

File: io-adapter.ts in package

  public bindMessageHandlers(
    socket: Socket,
    handlers: MessageMappingProperties[],
    transform: (data: any) => Observable<any>,
  ) {
  
  // THE BELOW CODE GETS EXECUTED EVERY TIME. 
    const disconnect$ = fromEvent(socket, DISCONNECT_EVENT).pipe(
      share(),
      first(),
    );

    handlers.forEach(({ message, callback }) => {
      const source$ = fromEvent(socket, message).pipe(
        mergeMap((payload: any) => {
          const { data, ack } = this.mapPayload(payload);
          return transform(callback(data, ack)).pipe(
            filter((response: any) => !isNil(response)),
            map((response: any) => [response, ack]),
          );
        }),
        takeUntil(disconnect$),
      );
      source$.subscribe(([response, ack]) => {
        if (response.event) {
          return socket.emit(response.event, response.data);
        }
        isFunction(ack) && ack(response);
      });
    });
  }

The default max listeners allowed by node process is 10 if you do not override the max listeners. Those of you who are facing this issue can do the following in your main entry file to fix the warning but you would have to calculate based on number of events your client is subscribing. If your events are dynamic in nature then by overtaking the socket and setting the maxlistenrs through the setMaxListeners function on the socket is the way to go.

File: main.ts

// your other imports like nest and etc
import { EventEmitter } from 'events';

/**
 * Setting max listeners to 15
 * It appears every message gateway added, adds a new listener to "disconnect" event. Needs investigation in the nestjs
 * codebase for platform socket.io
 */
(EventEmitter.prototype as any)._maxListeners = 15;

function bootstrap() {
// your main nestjs application creation code 
const app = await NestFactory.create<NestExpressApplication>(AppModule);
...
}

bootstrap();

I will look at the problem in detail in the main code of the package and try to see if I can generate a pull request. The main solution is to move the disconnect observable outside the handler function and keep using that with share action.

This is a very dangerous bug I would say as it is leading to memory leaks especially for cases where events subscribed are dynamic in nature as it exponentially escalates with number of websocket connection made. For those who have limited events like under 20 it is not going to impact as much but still dangerous none the less.

maxgaurav avatar Feb 23 '23 12:02 maxgaurav

Bump as this issue still exists in the current version of NestJS and those hacks are fine, but I'd love to see a final fix for this 🥹

axotion avatar Mar 29 '23 07:03 axotion

I am also experiencing this issue. As a solution, I'm considering centralizing the gateway in a single file, which will act as a router and call functions from other modules. I understand this could potentially complicate code maintenance, but it might prevent a decrease in app performance or memory leak.

luismariotti1 avatar Jun 03 '23 23:06 luismariotti1

As maxgaurav points out, the problem lies in the io-adapter. I've submitted a PR requesting some changes to solve this issue. If you could validate it, I would appreciate it. PR

lponciolo avatar Aug 03 '23 12:08 lponciolo

Hello. Any updates and plans to fix this? Or at least update documentation on the main page how to increase limit and that multiple gateways is not recommended.

lexarion1 avatar Feb 28 '24 20:02 lexarion1