Add `pending` property to indicates need ZMQ_POLLOUT… #174

Merged
merged 30 commits into from Feb 14, 2014

Conversation

Projects
None yet
9 participants
@soplwang
Contributor

soplwang commented Feb 1, 2013

When use 'PUSH/PULL' type sockets, if 'SNDHWM' exceeds, then _flush()
will not write the messages to underly zeromq socket but buffer them.
When underly socket queue became drain, we need _flush these buffered
message. But current version doesn't do this...

BTW: Added support for zmq3.x TCP keepalive.

xla and others added some commits Oct 23, 2012

Added link to zmq installation instructions
Otherwise error may occur during build time:

  CXX(target) Release/obj.target/binding/binding.o
../binding.cc:28:10: fatal error: 'zmq.h' file not found
         ^
1 error generated.
make: *** [Release/obj.target/binding/binding.o] Error 1
Ron Korving
Merge pull request #158 from freehaha/master
ZMQ_SNDHWM and ZMQ_RCVHWM should use type of int instead of uint64_t
Ron Korving
Merge pull request #166 from mscdex/backcompat
node v0.9.4+ compatibility
Ron Korving
Merge pull request #162 from ronkorving/master
ZMQ_LAST_ENDPOINT for automatic port assignment
Ron Korving
Merge pull request #156 from xla/master
Addition of XPUB/XSUB sockets
Ron Korving
Merge pull request #159 from bobrik/patch-1
Added link to zmq installation instructions
Ron Korving
Merge pull request #169 from datacratic/hanging-test-cases-on-linux
fixed hanging testcases on fast Linux machines
Ron Korving
Update README.md
Fixed a typo
Ron Korving
Update README.md
Fixed the TravisCI image on the readme.
@soplwang

This comment has been minimized.

Show comment Hide comment
@soplwang

soplwang Feb 1, 2013

Contributor

If using SNDHWM, then when peer consumed messages, it will use send_activate_write() to handshake [refs: zeromq3-x/src/pipe.cpp::read()]. This packet will active our Socket::UV_PollCallback. And we use zmq_poll(), this method will process handshake packet first, and then, our POLLOUT has a chance to active...

Contributor

soplwang commented Feb 1, 2013

If using SNDHWM, then when peer consumed messages, it will use send_activate_write() to handshake [refs: zeromq3-x/src/pipe.cpp::read()]. This packet will active our Socket::UV_PollCallback. And we use zmq_poll(), this method will process handshake packet first, and then, our POLLOUT has a chance to active...

binding.cc
+ case 34:
+ case 35:
+ case 36:
+ case 37:

This comment has been minimized.

Show comment Hide comment
@ronkorving

ronkorving Feb 4, 2013

Collaborator

Could you add comments to describe what these flags represent? (or just use the constants themselves)

@ronkorving

ronkorving Feb 4, 2013

Collaborator

Could you add comments to describe what these flags represent? (or just use the constants themselves)

This comment has been minimized.

Show comment Hide comment
@soplwang

soplwang Feb 4, 2013

Contributor

ok~

@soplwang

soplwang Feb 4, 2013

Contributor

ok~

test/test.socket.hwm.3-x.js
+ , should = require('should')
+ , sock = zmq.socket('req');
+
+if (zmq.version >= '3.0.0') {

This comment has been minimized.

Show comment Hide comment
@ronkorving

ronkorving Feb 4, 2013

Collaborator

This is probably not how you want to test for versions.
Have a look at https://github.com/JustinTulloss/zeromq.node/blob/master/test/test.exports.js
These constants are already being tested for.

@ronkorving

ronkorving Feb 4, 2013

Collaborator

This is probably not how you want to test for versions.
Have a look at https://github.com/JustinTulloss/zeromq.node/blob/master/test/test.exports.js
These constants are already being tested for.

This comment has been minimized.

Show comment Hide comment
@soplwang

soplwang Feb 4, 2013

Contributor

yeah, i'll use semver to do version check.

The reason is: the master's SetSockOpt() on ZMQ_SNDHWM and ZMQ_RCVHWM is using SetSockOpt<char*>() template, that makes sock.setsockopt(zmq.ZMQ_SNDHWM, 1) fail...

@soplwang

soplwang Feb 4, 2013

Contributor

yeah, i'll use semver to do version check.

The reason is: the master's SetSockOpt() on ZMQ_SNDHWM and ZMQ_RCVHWM is using SetSockOpt<char*>() template, that makes sock.setsockopt(zmq.ZMQ_SNDHWM, 1) fail...

test/test.socket.messages.lateconnect.js
+} else {
+ push.setsockopt(zmq.ZMQ_HWM, 1);
+ pull.setsockopt(zmq.ZMQ_HWM, 1);
+}

This comment has been minimized.

Show comment Hide comment
@ronkorving

ronkorving Feb 4, 2013

Collaborator

I think it makes sense to use semver version checking here, like in the test I referred to above.

@ronkorving

ronkorving Feb 4, 2013

Collaborator

I think it makes sense to use semver version checking here, like in the test I referred to above.

This comment has been minimized.

Show comment Hide comment
@soplwang

soplwang Feb 4, 2013

Contributor

ok

@soplwang

soplwang Feb 4, 2013

Contributor

ok

Use TCP_KEEPALIVE_* constants instead of raw number.
And with improves on unit tests to consist of.
@ronkorving

This comment has been minimized.

Show comment Hide comment
@ronkorving

ronkorving Feb 14, 2013

Collaborator

Looks clean, but I'm not qualified (familiar with this feature in ZMQ) to review it fully. Any takers?

Collaborator

ronkorving commented Feb 14, 2013

Looks clean, but I'm not qualified (familiar with this feature in ZMQ) to review it fully. Any takers?

This comment has been minimized.

Show comment Hide comment
@ghost

ghost May 10, 2013

This merge seems to be important to prevent memory leaks when sending messages at high frequency from push to pull.
Currently our Push Service produces a Leak of round about 40Mb/Week.
Is there a Workaround to avoid this behavior?(until the merge is done)
Thanks in advance.

ghost commented May 10, 2013

This merge seems to be important to prevent memory leaks when sending messages at high frequency from push to pull.
Currently our Push Service produces a Leak of round about 40Mb/Week.
Is there a Workaround to avoid this behavior?(until the merge is done)
Thanks in advance.

@ronkorving

This comment has been minimized.

Show comment Hide comment
@ronkorving

ronkorving Aug 6, 2013

Collaborator

Can anyone (perhaps @soplwang ?) confirm whether current master solves this issue?

Collaborator

ronkorving commented Aug 6, 2013

Can anyone (perhaps @soplwang ?) confirm whether current master solves this issue?

@kkoopa

This comment has been minimized.

Show comment Hide comment
@kkoopa

kkoopa Aug 30, 2013

Contributor

The added test case passes at least.

Contributor

kkoopa commented Aug 30, 2013

The added test case passes at least.

@ronkorving ronkorving merged commit 6993565 into JustinTulloss:master Feb 14, 2014

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment