-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix drain leak #1401
fix drain leak #1401
Conversation
@@ -175,7 +175,7 @@ function sendPacket (client, packet, cb) { | |||
debug('sendPacket :: writing to stream') | |||
const result = mqttPacket.writeToStream(packet, client.stream, client.options) | |||
debug('sendPacket :: writeToStream result %s', result) | |||
if (!result && cb) { | |||
if (!result && cb && cb !== nop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code (the way sendPacket
handles writeToStream
return values) is built on a number of false assumptions:
- It assumes that
writeToStream
will only returnfalse
if the packet was queued and is waiting to send. This is wrong.writeToStream
can also fail if packet validation fails (inmqtt-packet/writeToStream.js
) If this happens,sendPacket
will wait for the stream to drain, then call the callback with no error. - If the stream is full, and
writeToStream
returns false, it waits for all packets to send before calling any of their callbacks. Once all packets are sent, thedrain
event fires, and all callbacks get called at once. If the sending pattern prevents the stream from draining completely, then the callbacks will never be called (or they might be called long after the packet was actually sent).
3, This assumes that there can only be 1000 packets waiting to send. After that, the maxListeners on thedrain
event overflows. I'm not sure how well the library handles this error (Does it raise an exception? If so, who catches it?)
A better fix would be to pass the callback into the stream's write
method so it gets called when the packet is actually sent. At least this is how TLSSocket.write
behaves. I don't know if this applies to all of the stream implementations that mqtt.js supports. This would require an update to the mqtt-packet
library to support an additional callback
parameter to the writeToStream
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus, packetsend
is emitted before the packet is actually sent (if the packet is ever actually sent). Is that what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened an issue on this in #1403
@redboltz I'm not able to request a review from you for some reason, but pinging you in case you have any feedback here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
…eak github workflow
Fix a regression introduced by #1301. The
cb = cb || nop
line in_sendPacket
was adding a callback where none existed before. This changed the behavior ofsendPacket
(without the underscore) by adding a listener to thedrain
event, and you can quickly overflow the max # of listeners if you're trying to publish faster than the stream will allow.