Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closing client socket with auto reconnect enabled #33

Open
enbyted opened this issue Jan 1, 2015 · 8 comments
Open

Closing client socket with auto reconnect enabled #33

enbyted opened this issue Jan 1, 2015 · 8 comments

Comments

@enbyted
Copy link

enbyted commented Jan 1, 2015

When closing client socket with 'reconnect' enabled using socket.end() scheduled reconnect function will crash the application (~line 322 in nssocket.js, when removing listeners from socket).
If I try socket.destroy() reconnect function would recreate the internal socket and connect to server, but it shouldn't, application requested closing of socket.

@genediazjr
Copy link
Member

any chance you could provide us a snippet of your code?

@enbyted
Copy link
Author

enbyted commented Jan 2, 2015

Currently I'm working on a big project with really short deadline, that's how I ran into it, but I'll make a test code when I'll get some time (probably in few days). Just a quickie how to reproduce:

  • Create a nssocket with 'reconnect' option enabled and I believe it was type set to 'ipv4' (just like in example)
  • Let it connect
  • Call socket.end() - this will throw a exception after some time
    OR - Call socket.destroy() - this will reopen the internal socket after some time

As I said earlier I'll write some code when I'll get some time, if it'll still be necessary.
I got around that issue by writing custom reconnect logic on application level.

@genediazjr
Copy link
Member

Try putting an on 'error' handler like below

socket.on('error', function (err) {
  log.error('client error', err);
});

@genediazjr
Copy link
Member

@enbyted any updates? tnx!

@enbyted
Copy link
Author

enbyted commented Jan 15, 2015

Thanks for the ping, I forgot about that in the mass of work lately, here is a failing test code:

var net = require('net'),
    nssocket = require('nssocket');

net.createServer(function (socket) {
  console.log('client connected');
}).listen(8345);

var socket = new nssocket.NsSocket({
  reconnect: true,
  type: 'tcp4',
});

socket.on('start', function () {
  console.dir('start');
});

socket.on('error', function (err) {
  console.log('client error', err);
});

socket.connect(8345);
setTimeout(function() {
    socket.end();
}, 250);

So, that will crash with:

<workspace>/node_modules/nssocket/lib/nssocket.js:323                                                                                      
    self.socket.removeAllListeners();                                                                                                                      
                ^                                                                                                                                          
TypeError: Cannot call method 'removeAllListeners' of null                                                                                                 
    at doReconnect (<workspace>/node_modules/nssocket/lib/nssocket.js:323:17)                                                              
    at tryReconnect [as _onTimeout] (<workspace>/node_modules/nssocket/lib/nssocket.js:351:5)                                              
    at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)    

If you change the socket.end() to socket.destroy() it will not crash, but it'll reconnect without calling on.start (you'll see on program's output one 'start' ond two 'client connected')

So, there is no way to close an autoreconnect socket.

Also, I'm using version '"0.5.3" from NPM repo, I think that's the newest.

@genediazjr genediazjr reopened this Jan 15, 2015
enbyted added a commit to enbyted/nssocket that referenced this issue Feb 1, 2015
Reconnection logic is now disabled when disconnect is by request (call to .end() or .destroy()).
@nucleardreamer
Copy link
Member

+1 for merging this. @enbyted can you make a pull request?

@enbyted
Copy link
Author

enbyted commented Jul 7, 2015

@nucleardreamer There already is one (#34).

@nucleardreamer
Copy link
Member

Oh awesome, thank you for writing that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants