JustinTulloss / zeromq.node

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

Socket.pause() only pauses emitting of 'message' events, not actual retrieval of messages #480

Open elyscape opened 8 years ago

elyscape commented 8 years ago

When calling pause() on a pull socket, messages continue to be retrieved in the background. The only difference is that no 'message' events are fired until resume() is called on the socket, at which point events are emitted for all previously-retrieved messages. This is unexpected behavior and should probably either be documented or fixed to behave as expected.

reqshark commented 8 years ago

Unfortunately I think you're right, I had a feeling that might be the case :(

reqshark commented 8 years ago

here just testing an emit() or read() call at the JS layer, not the actual socket library's recv.

  it('should not emit messages after pause()', function(done){
      var push = zmq.socket('push')
        , pull = zmq.socket('pull');

      var n = 0;

      pull.on('message', function(msg){
        if(n++ === 0) {
          msg.toString().should.equal('foo');
        }
        else{
          should.not.exist(msg);
        }
      });

https://github.com/JustinTulloss/zeromq.node/blob/master/test/socket.push-pull.js#L42-L73

also wtf is should.not.exist(msg);? Undefined? Null? exist is not a type, IMO nor a convention in JS

kroeders commented 8 years ago

I think should.not.exist is part of the mocking framework...

achimnol commented 8 years ago

Could we have unref() / ref() APIs just like the intrinsic Net.Socket objects? I think we could implement that by passing poll_handle_ of binding.cc to uv_unref() and uv_ref() functions.

reqshark commented 8 years ago

Could we have unref() / ref() APIs just like the intrinsic Net.Socket objects?

sounds interesting, what's the use case?

achimnol commented 8 years ago

There may be cases that we want to skip checking pending RX/TX data. Just like the vanilla node sockets, one would want terminate the main program without explicitly closing sockets. I think this is particularly useful for ZMQ since it provides "asynchronous" semantics by its design, such as PUB/SUB sockets where the "actual" OS-level socket connection does not matter.

For me, I'm implementing an event-loop waiter as a C++ addon to nodejs that repeatedly calls uv_run() until there is no more pending events to execute and wait until arbitrary user code finishes, where I do not have any prior information on what callbacks the input code would generate. (This is what exactly the main loop of nodejs itself does to decide if it should terminate or not, for the entire program. Now I want to use the same blocking mechanism for just a specific region of codes.) To make this work, I need to temporarily "unref" all existing callbacks such as timers and sockets.

BTW, my feature request may not be aligned with this thread, though I have crashed here after trying pause() for my purpose. So I've created a new issue: #502.

royaltm commented 7 years ago

I've found another bug concerning pause/resume. Socket.pause() only pauses AFTER flushing messages queued in the binding. User however might call pause() within 'message' handler but it won't stop flushing messages until the internal binding's queue is empty.

I think this line: https://github.com/JustinTulloss/zeromq.node/blob/7e2d99b33864fb54acec01b19542805257ffafa1/lib/index.js#L640 should be replaced with:

return !this._paused;

Thanks to this another _flushRead() will NOT be called from _flushReads() until user calls resume();