JustinTulloss / zeromq.node

Node.js bindings to the zeromq library
MIT License
1.65k stars 286 forks source link

Fix socket stops receiving messages (#558) #575

Closed hhamilto closed 7 years ago

hhamilto commented 7 years ago

If error is thrown in message event handler and error event handler is attached to socket, _flushRead is called until no more messages are read.

hhamilto commented 7 years ago

Does anyone know why this build fails on OSX but not Ubuntu? I'm developing on Ubuntu, and not able to reproduce.

ronkorving commented 7 years ago

A simpler solution would've been to wrap while (this._flushRead()); into a try/finally where in the finally block you put this._isFlushingReads = false;, no?

In any case, I very much appreciate your contribution :)

hhamilto commented 7 years ago

The underlying issue is that when we receive a message, we need to call Socket::Readv once again after reading the message for libuv to properly call the poll callback and notify us of new messages.

I'm doing a little guess work, but I believe that calling zmq_getsockopt(socket->socket_, ZMQ_EVENTS, &events, &events_size) in Socket::Readv unsets the UV_READABLE event. If this event isn't unset, then lib_uv will never notify us of incoming messages.

hhamilto commented 7 years ago

@ronkorving to more directly address your thoughts on the fix: We need to make sure to call readv one final time after reading any queued messages. I changed _flushRead so that it returned true (signifying that it had read a message and emitted the message event) even when the message event handler threw an error. This is different than the functionality that was in place before, where the return code from _flushRead that we use in _flushReads was undefined when _flushRead threw an error.

Does that seem right, or perhaps there is a simpler fix/underlying issue that I'm not aware of?

Thanks for feed back and review!

ronkorving commented 7 years ago

I see your point. But now if the read fails with an exception, you also return true, which is probably not what we want. In that case, I think we need to return false. Reading the old code again though, it's the same there.

ronkorving commented 7 years ago

Thank you, for your contribution and your patience :)

hhamilto commented 7 years ago

Happy to help. Happy Halloween :)