fix: problem with publish callback invoked twice

[ogis-yamazaki]

User callback is executed twice if the received PUBACK indicates failure (by reason code).
Related issue: #1511

The following modification prevents the callback from invoked twice.

• If resonCode in PUBACK to failure, set the cb variable to NULL after the callback is invoked.
• When deleting a message from the store, do not invoke the callback if cb is null

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.53 🎉

Comparison is base (d71b000) 85.74% compared to head (d226163) 86.28%.

❗ Current head d226163 differs from pull request most recent head b5ffdd8. Consider uploading reports for the commit b5ffdd8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1635      +/-   ##
==========================================
+ Coverage   85.74%   86.28%   +0.53%     
==========================================
  Files          13       13              
  Lines        1249     1254       +5     
==========================================
+ Hits         1071     1082      +11     
+ Misses        178      172       -6     

Impacted Files	Coverage Δ
lib/client.js	91.16% <100.00%> (+0.73%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[robertsLando]

Thanks for the PR @ogis-yamazaki ! Would you mind to add a unit test that covers this?

[robertsLando]

Suggested change

	cb = null
	// prevent invoking callback twice when deleting message from store #1511
	cb = null

[ogis-yamazaki]

@robertsLando

Added qos 1 and qos2 tests.

When an error occurs in PUBREC, added a process to remove the message from the outgoingStore.

[robertsLando]

LGTM

[robertsLando]

By checking the code on my side I saw that this:

delete this.outgoing[messageId]
      this.outgoingStore.del(packet, cb)
      this.messageIdProvider.deallocate(messageId)
      this._invokeStoreProcessingQueue()

Is used 3 times in the code. Would you mind to create a dedicated function for that?

I see we also have a method removeOutgoingMessage, it doesn't call _invokeStoreProcessingQueue and messageIdProvider.deallocate maybe that's an error too?

[ogis-yamazaki]

@robertsLando

Is used 3 times in the code. Would you mind to create a dedicated function for that?

OK

I see we also have a method removeOutgoingMessage, it doesn't call _invokeStoreProcessingQueue and messageIdProvider.deallocate maybe that's an error too?

As you said, additional processing was needed.

In removeOutgoingMessage, needs to call messageIdProvider.deallocate.
If not called, the messageId will be in use forever.

_invokeStoreProcessingQueue must also be called.
Normally, _invokeStoreProcessingQueue is called when a PUBACK or similar event occurs, but removeOutgoingMessage will not pass through it.
The _storeProcessingQueue may remain unprocessed.

The _invokeStoreProcessingQueue function also seems to have an implementation problem.

There is a problem if _invokeStoreProcessingQueue is called while the store is resending(_storeProcessing = true).
Specifically, Message overtaking may occur.
It is necessary to modify the content of _storeProcessingQueue`` so that it is not processed during resend(_storeProcessing = true).

[robertsLando]

Would you mind to also fix those errors or do you prefer to do that in a separeted PR?

[ogis-yamazaki]

@robertsLando

Would you mind to also fix those errors or do you prefer to do that in a separeted PR?

Fixed in this PR.

[robertsLando]

LGTM, now everything is much cleaner. Thanks @ogis-yamazaki 🙏🏼