[bug] Client error publishing two messages during same tick with overlapping subs

[nfrolov]

System Information

• Aedes: 0.42.6
• NodeJS: 12.13.1
• OS: macOS 10.14.6
• Arch: x86_64

Describe the bug If two QoS 1/2 messages with same topic are published during single tick and client has overlapping subscriptions, then client error no such packet is emitted and client is disconnected.

To Reproduce Steps to reproduce the behavior:

1. Checkout aedes
2. Copy test into test directory using wget -P test/ https://gist.githubusercontent.com/nfrolov/9c11d8e3e2c797eb59c0f6df0e0aaa2c/raw/c0a6c4fb16c963d821a158f3670b293611138ce8/same-tick-overlapping-subs-qos-1-clean-false.js
3. Run test node test/same-tick-overlapping-subs-qos-1-clean-false.js

Expected behavior Both messages are delivered successfully.

Additional context Last working aedes version is 0.39.0. Test is broken starting https://github.com/moscajs/aedes/commit/d7a1f82cb73261e4753303071fdfaaa196e20328.

Naive fix would be to wrap outgoingClearMessageId with setImmediate as in https://github.com/nfrolov/aedes/commit/ebf53b8e8e1c6409085efde1bd66206717129ad5.

I do not completely understand what is happening under the hood, but in case of messages published by broker and overlapping subscriptions outgoingClearMessageId is called on packets which do not have messageId defined. If messageId is undefined, then aedes-persistence just deletes any first message without message id. It does not look like an expected behavior.

[robertsLando]

If messageId is undefined, then aedes-persistence just deletes any first message without message id

I think your fix could be a possible solution but I don't think it's correct to call outgoingClearMessageId when messageId is undefined.

Thoughts @mcollina ?

[robertsLando]

I think we are missing a new QoSPacket(packet) construtor in aedes.publish functions when qos is > 0.

Maybe here: https://github.com/moscajs/aedes/blob/master/aedes.js#L181 or https://github.com/moscajs/aedes/blob/4de90db8a0b32b3698d397f95b2403388df87217/aedes.js#L214?

[robertsLando]

Maybe I'm missing something here but in aedes-persistence outgoingEnqueue new Packet is called:

https://github.com/moscajs/aedes/blob/master/aedes.js#L254

And that will result in messageId to be undefined:

https://github.com/moscajs/aedes-packet/blob/master/packet.js#L17

So there should be no reason to call outgoingClearMessageId without a previous outgoingUpdate

[mcollina]

I currently have no time to look into this.

[robertsLando]

@nfrolov Could you please send a pr with the setImediate fix and the test?