search

Rantanen / node-mumble

Mumble client in Node.js
MIT License
155 stars 48 forks source link

Exception in examples/wrapper.js #21

Closed Prior99 closed 9 years ago

Prior99 commented 9 years ago

(node) warning: possible EventEmitter memory leak detected. 11 permissionQuery listeners added. Use emitter.setMaxListeners() to increase limit. Trace at EventEmitter.addListener (events.js:179:15) at EventEmitter. (/home/prior/Projects/node-mumble/lib/MumbleWrapper.js:33:20) at EventEmitter.emit (events.js:110:17) at EventEmitter.addListener (events.js:150:10) at EventEmitter.Channel (/home/prior/Projects/node-mumble/lib/Channel.js:18:18) at EventEmitter.MumbleWrapper._newChannel (/home/prior/Projects/node-mumble/lib/MumbleWrapper.js:113:19) at EventEmitter.MumbleWrapper._channelState (/home/prior/Projects/node-mumble/lib/MumbleWrapper.js:156:47) at EventEmitter.emit (events.js:107:17) at EventEmitter.MumbleConnection._processMessage (/home/prior/Projects/node-mumble/lib/MumbleConnection.js:403:10) at EventEmitter.MumbleConnection._processData (/home/prior/Projects/node-mumble/lib/MumbleConnection.js:358:14) (node) warning: possible EventEmitter memory leak detected. 11 permissionQuery listeners added. Use emitter.setMaxListeners() to increase limit. Trace at EventEmitter.addListener (events.js:179:15) at EventEmitter.Channel (/home/prior/Projects/node-mumble/lib/Channel.js:18:18) at EventEmitter.MumbleWrapper._newChannel (/home/prior/Projects/node-mumble/lib/MumbleWrapper.js:113:19) at EventEmitter.MumbleWrapper._channelState (/home/prior/Projects/node-mumble/lib/MumbleWrapper.js:156:47) at EventEmitter.emit (events.js:107:17) at EventEmitter.MumbleConnection._processMessage (/home/prior/Projects/node-mumble/lib/MumbleConnection.js:403:10) at EventEmitter.MumbleConnection._processData (/home/prior/Projects/node-mumble/lib/MumbleConnection.js:358:14) at Object.callback (/home/prior/Projects/node-mumble/lib/MumbleConnection.js:804:18) at MumbleSocket._checkReader (/home/prior/Projects/node-mumble/lib/MumbleSocket.js:125:12) at MumbleSocket.read (/home/prior/Projects/node-mumble/lib/MumbleSocket.js:55:43)

Prior99 commented 9 years ago

@Rantanen Can you take a look at it? Or should I?

Rantanen commented 9 years ago

Wonder if we should get rid of the internal event emitter usage instead of just raising/disabling the maximum listener check..

This isn't really an exception as such, it's just EventEmitter warning us that "you've now added lots of event listeners. Are you really sure you want this?"

However our current implementation has pretty much each User/Channel listening to their own events thus the amount of event listeners depends on the amount of users/channels. So for a perfect solution we'd need to alter the setMaxListeners amount depending on the amount of users/channels or then just disable it by setting it to 0. The first alternative is difficult and the second alternative.. well.. disables one of the event leak failsaves.

Prior99 commented 9 years ago

Isn't it the MumbleWrapper listening for the events of the User and Channel and forwarding it? I implemented it this way so you can listen for an event on a specific user as well as on the wrapper for a general event.

Rantanen commented 9 years ago

An alternative to the internal event emitter usage would be registering a callback on the connection for MumbleWrapper that the connection uses to notify the wrapper about the events. Then instead of the User and Channel listening for events, the MumbleWrapper would handle them and delegate them forward to User or Channel.

It's a bit more work to implement but in the end I think it would be cleaner for users of the library.

Rantanen commented 9 years ago

Oh. You're right. It was already using half of what I suggested.

channelState, etc. are registered per wrapper and then delegated to the Channel/User. It's still using events internally but just one per item. It's the permission code of mine which registers the on-handler in Channel constructor which causes one handler per Channel. Fixing that one.