do not use events internally #171

Open
rauchg opened this Issue May 6, 2013 · 8 comments

Comments

Projects
None yet
5 participants
Contributor

rauchg commented May 6, 2013

like once('close') in server.js, since a user could call removeAllListeners('close')

nullbox commented Feb 8, 2014

I wrote a small failing test case for this
https://github.com/nullbox/engine.io/compare/master...issue-171

Does this issue belong in engine.io-client?

Contributor

rauchg commented Feb 9, 2014

Yes

brishin commented Feb 9, 2014

Follow up: it seems like very little code depends on events internally in the client code: https://github.com/LearnBoost/engine.io-client/blob/master/lib/socket.js

@brishin yeah, looks like 5 instances each of .on() and .once(). There's also an instance in lib/util.js and many more in the various lib/transports

brishin commented Feb 9, 2014

@davidhcummings Right, but there are only 3(?) instances of using the Emitter on the Socket object.
I don't think that the lib/transports are important because users shouldn't be messing with the transport objects anyway.

Contributor

konstantinzolotarev commented Feb 16, 2017 edited

Faced with same issue.
I understand that it's not a real issue but mistake in code that based on engine.io.
But if I need to remove all close listeners from socket using socket.removeAllListeners('close') it will cause a memory leak into application. Because server.clients would be never cleaned and connection amount wouldn't reduce after some socket closes connection.

Because of this: https://github.com/socketio/engine.io/blob/master/lib/server.js#L334
This block would never be reached

Contributor

konstantinzolotarev commented Feb 16, 2017

Might be it makes sense to split internal events and events that developers could rely on ?
For example for internal use could be created internal-close event but for usage normal close event would be fired ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment