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

Setting SNDHWM has no effect #152

Closed
interpretor opened this issue Jun 7, 2017 · 5 comments
Closed

Setting SNDHWM has no effect #152

interpretor opened this issue Jun 7, 2017 · 5 comments

Comments

@interpretor
Copy link
Member

Setting the SNDHWM currently has no effect to the public api.
The zmq sendv function returns true while the socket doesn't block, otherwise it returns false.
The index.js creates a BatchList and stores all outgoing message buffers as an OutBatch in the list. The BatchList wraps the sendv function, and the outgoing messages, that could not be sent due to the blocking socket, remain without any event or return value in the list.
Why there is another queue implemented in JS around the zmq sockets?
With this behavior I cannot say if a message has been delivered or not. Or am I missing here something, and I can tell in some way if the message has been delivered?

The point is that I would need to know which messages could be delivered after a period of time, so closing the socket doesn't destroy the messages (and therefore they will be lost forever).

@interpretor
Copy link
Member Author

I may implement an option to the BatchList, so we can set a high water mark. Would this be okay?
Or should we rethink the queuing mechanism (and maybe only use the native queue)?

@rgbkrk
Copy link
Member

rgbkrk commented Jun 26, 2017

Unless someone else speaks up, this is largely your call. I trust in your judgement as we bring this module further into the future. 😄

@rolftimmermans
Copy link
Member

Having looked at the C++ code recently myself the additional queueing also struck me as odd. I found this commit JustinTulloss/zeromq.node@4d16140 that seems to have introduced this. There is a claim of big performance improvements.

I guess that executing zmq calls synchronously could cause delays in the Node event queue, but I'm not sure which scenarios were targeted by the commit above.

I definitely feel that the additional queueing is not very transparent and could lead to different behaviour compared to other zmq bindings. It might be worth rethinking this implementation and seeing if it could be changed to something simpler that is also performant.

@ronkorving
Copy link

@rolftimmermans I'm all for rethinking that.

For context: One of the main reasons I believe that change improved performance was because under heavy traffic scenarios, there would be way less jumps between JS and native.

@rolftimmermans
Copy link
Member

The latest 6.0 beta fully supports high water marks. One way this has been addressed is to change the API so that sending/receiving returns a promise which will be resolve only after a message was successfully queued on the ZMQ socket. Settings such as timeouts and high water marks may cause queueing to fail and the promise to be rejected. With the new API all features of ZMQ should work as expected in Node.js. See #189 for more of the reasoning behind the new API.

If you run into any problems feel free to report it here or in a new issue.

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

4 participants