Sub socket stops after message handler throws an exception

[n-riesco]

I've come across an issue similar to #442 still present in v2.5.13. This time happens with a sub socket.

Here's a script that demonstrates the issue:

var zmq = require('zmq');

s_pub = zmq.socket('pub');
s_pub.bindSync('tcp://*:45000');

s_sub = zmq.socket('sub');
s_sub.connect('tcp://localhost:45000');
s_sub.subscribe("");

s_sub.on('error', function(x) { 
    console.log("error handler: ", x); 
});

s_sub.once('message', function(x) {
    console.log("sub recv:", x.toString(), " throwing now");
    throw "sorry";
});

console.log("-- msg1");
s_pub.send("msg1");

setTimeout(function() {
    s_sub.once('message', function(x) {
        console.log("sub recv:", x.toString(), " 1st after throw");
    });
    console.log("-- msg2");
    s_pub.send("msg2");
}, 500);

setTimeout(function() {
    s_sub.once('message', function(x) {
        console.log("sub recv:", x.toString(), " 2nd after throw");
    });
    console.log("-- msg3");
    s_pub.send("msg3");
}, 1000);

The output I get using zmq v2.5.13 is:

-- msg1
sub recv: msg1  throwing now
error handler:  sorry
-- msg2
-- msg3

What I'd expect is:

-- msg1
sub recv: msg1  throwing now
error handler:  sorry
-- msg2
sub recv: msg2  1st after throw
-- msg3
sub recv: msg3  2nd after throw

[hhamilto]

I can also confirm that this happens on zmq v4.1.5 using node v6.3.1

I think that expected behavior would be that the program would crash after the error is thrown, like it does when an error is thrown in an event handler for a vanilla EventEmitter:

ee = new (require('events'))
ee.on('test', ()=>{
  throw "sorry"
})
ee.emit('test')

yeilds:

/home/hurricane/projects/scratch/scratch.js:5
    throw "sorry"
    ^
sorry

Instead, the error is never logged, and message events stop getting emitted, like @n-riesco described.

@JustinTulloss would you accept a PR?

[rgbkrk]

@ronkorving is the primary maintainer. Go for the PR @hhamilto!

[hhamilto]

@rgbkrk Thanks for the info!

Regarding my assumption that errors thrown from the message event handler should bubble up normally and halt execution: It looks like we intentionally prevent this from happening inside the Socket.prototype._flushReads method:

try {
    received = this._flushRead();
} catch (error) {
    this._isFlushingReads = false;
    this.emit('error', error); // can throw
    return;
}

Instead, as you can see in the above excerpt, we catch the errors, and emit them as error events on the socket. While this seems odd to me, it also seems like we should not break this existing behavior. We should however, still fix the underlying issue.

The underlying issue is that after the error is handled, we never call _flushRead again, like we normally would. For some reason, calling _flushRead (and it in turn calling Readv in the binding) resets the UV_READABLE event on the socket, and allows libuv to notify us of new messages. currently, the UV_READBALE event is never unset and messages simply queue up without the _flushReads method ever being called again. _flushReads is normally called when the libuv detects the underlying socket is readable and calls our UV_PollCallback callback. I should have a PR to fix this issue up in a day or two, which might make the issue clearer.

[hhamilto]

PR here: #575