No callback on writeToStream?

[YoDaMa]

In the writeToStream() code there is no use of callbacks or checking the return value on write() calls. For instance here on connect:

mqtt-packet/writeToStream.js

Line 216 in 23774e7

stream.write(protocol.CONNECT_HEADER)

As a result the only way to see if the packet has failed to send is to listen for an event on the stream.

In aedes, there is an implicit assumption that whenever writeToStream() is called, the buffer highwatermark will be hit and then once the buffer drains 'drain' will be called. See the aedes write.js file: https://github.com/moscajs/aedes/blob/main/lib/write.js

What is the assumed behavior for writeToStream()? Do we assume that whenever we call it, the highwatermark will be hit and subsequently the drain will be called? Or is that a false assumption being made by the aedes code? What is the best way to check if the call succeeded to write the data to the stream or if it failed?

@mcollina do you have insights into this since you helped write mqtt-packet and the new aedes code?

[mcollina]

What is the assumed behavior for writeToStream()? Do we assume that whenever we call it, the highwatermark will be hit and subsequently the drain will be called? Or is that a false assumption being made by the aedes code?

Either drain or error will be emitted if writeToStream return false. There is no need to wait if it returns true.

What is the best way to check if the call succeeded to write the data to the stream or if it failed?

You don't want to wait for a confirmation that data is written to the stream. It has no real meaning of being actually sent on the wire, just that it has been received by the underlining layer. In fact, sending the data could fail even if the callback is invoked successfully.

The way Aedes does it is correct to achieve maximum throughput on a socket.

[vishnureddy17]

Thanks for the clarification @mcollina.

Follow-up question: What is the rationale behind the design of emitting errors on the passed-in stream outside of the internal stream context? My naive inclination would be that error should only be emitted internally in the stream context and not from the outside.

For context, I've been discussing with @YoDaMa offline.

[mcollina]

Follow-up question: What is the rationale behind the design of emitting errors on the passed-in stream outside of the internal stream context? My naive inclination would be that error should only be emitted internally in the stream context and not from the outside.

I don't understand the question. Some of the design of Node.js stream does not have a clear rationale, it just happened 🤦‍♂️😅.

[vishnureddy17]

Sorry my question wasn't clear. In mqtt-packet, if there is an error then stream.emit('error') is called. I would think it is bad practice to call emit('error') on the stream instance because it is not part of the public API of streams and is an internal thing. What is the reason for mqtt-packet to do this?

Example:

mqtt-packet/writeToStream.js

Line 109 in 23774e7

stream.emit('error', new Error('Invalid protocol version'))

[mcollina]

Oh! I forgot we were doing that in this module. This problem was fixed long ago in Node.js. We should be calling stream.destroy(err) instead.

Would you like to send a PR?