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

fix(socket): Fixes socket.use error packet which drops nodejs due to … #2772

Merged
merged 4 commits into from
Dec 1, 2016
Merged

fix(socket): Fixes socket.use error packet which drops nodejs due to … #2772

merged 4 commits into from
Dec 1, 2016

Conversation

serhiisol
Copy link
Contributor

@serhiisol serhiisol commented Nov 30, 2016

This PR fixes #2771

The kind of change this PR does introduce

  • a bug fix

Current behaviour

Socket.use middleware drops nodejs process if error thrown in next function

New behaviour

Socket.use middleware sends error packet correctly without dropping nodejs instance.

Other information (e.g. related issues)

Socket cannot emit error event, because default behavior of EventEmitter will drop nodejs instance.
Details: https://nodejs.org/api/events.html#events_error_events

@darrachequesne
Copy link
Member

Hi! Thanks for the PR. Actually, there seems to be an issue in the tests:

it('should pass errors', function(done){
  var srv = http();
  var sio = io(srv);

  srv.listen(function(){
    var socket = client(srv, { multiplex: false });

    socket.emit('join', 'woot');

    // + socket.on('error', function(err){
    // +   expect(err).to.be('Authentication error');
    // +  done();
    // + });

    sio.on('connection', function(socket){
      socket.use(function(event, next){
        next(new Error('Authentication error'));
      });
      socket.use(function(event, next){
        done(new Error('nope'));
      });

      socket.on('join', function(){
        done(new Error('nope'));
      });
      // - socket.on('error', function(err){
      // -   expect(err).to.be('Authentication error');
      // -   done();
      // - });
    });
  });
});

According to the docs:

Errors passed to middleware callbacks are sent as special error packets to clients.

Your first commit should be sufficient I think. Could you please update the tests, by renaming the 2nd socket?

@serhiisol
Copy link
Contributor Author

serhiisol commented Nov 30, 2016

Hi @darrachequesne, I noticed that after deep debugging, and I've updated PR with fixes. In the test clientSocket should listen to error event, but not server side one

@darrachequesne darrachequesne merged commit 1e31769 into socketio:master Dec 1, 2016
@serhiisol serhiisol deleted the socket-use-error-fix branch December 1, 2016 08:44
@darrachequesne darrachequesne added this to the 1.8.2 milestone Dec 11, 2016
dzad pushed a commit to dzad/socket.io that referenced this pull request May 29, 2023
* fix(socket): Fixes socket.use error packet which drops nodejs due to nuances of Nodejs' EventEmitter

* fix(socket): Fixes missing error event on socket

* fix(socket): test fix, should listen for clientSocket instead of server socket

* minor update
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

Successfully merging this pull request may close these issues.

Socket.use error drops server
2 participants