emit conn-close on connection end event. #40

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants

Hi Astro,

I found the node-xmpp is great most of the time. One thing I think we can improve is to detect socket disconnect and report to caller. I made a little change in connection.js and it seems working for me.
Usage example of to listen 'conn-close' event at Client.
cl = new xmpp.Client()
cl.on('conn-close', function() {
console.log('connection ends');
});

I think doing auto-reconnect directly at Client is also possible. Able to report disconnect looks a MUST to have to me.

Cheers,
George

contra commented Nov 4, 2011

Shouldn't it be 'disconnected' for consistency?

georgeye commented Nov 5, 2011

I can change the code to use 'disconnected' if needed.

Owner

astro commented Nov 14, 2011

Node's standard events are:

  • end when the connection has been closed on the remote side and there won't be any incoming traffic anymore. The local side can still send and should eventually send end() as well
  • close when the file descriptor has finally been closed

I would like to stick to those. Do they fit this case?

I updated to use 'close' event.

contra commented Dec 23, 2011

Any chance of this being pulled in?

Owner

astro commented Dec 23, 2011

Normal TCP streams emit end and close too. What is the reason for not sticking to the node.js conventions?

Owner

astro commented Dec 23, 2011

Oops, now I understand. Sorry for the delay.

noroute commented May 4, 2012

What's the status on this issue? It would be really useful to have events for closed connections...

Owner

astro commented Jun 22, 2012

We do have auto-reconnect in place.

The master branch is a place of big restructuring right now. I will proxy close from the socket eventually. Can you use the end event in the meanwhile?

Owner

lloydwatkin commented May 18, 2013

Hi guys, I'm going to close this as the issue is around a year old (I'm triaging all the old PRs/Issues). If this is still an issue let me know and lets get a new clean pull request in for it. Thanks, Lloyd.

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