The way sendPacket handles writeToStream return values is built on a number of false assumptions

[vishnureddy17]

This code (the way sendPacket handles writeToStream return values) is built on a number of false assumptions:

1. It assumes that writeToStream will only return false if the packet was queued and is waiting to send. This is wrong. writeToStream can also fail if packet validation fails (in mqtt-packet/writeToStream.js) If this happens, sendPacket will wait for the stream to drain, then call the callback with no error.
2. 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, the drain 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 the drain 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.

Plus, packetsend is emitted before the packet is actually sent (if the packet is ever actually sent). Is that what we want?

Originally posted by @BertKleewein in #1401 (comment)

[dhyeyyyy]

hey- has there been any activity on this? the client crashes when we don't dispatch a message at a frequency beyond the keepAlive value and renders our outgoing store useless.

[robertsLando]

@dhyeyyyy MQTTjs version? Please try using latest