Cancel acceptance of publish messages about gomqtt

[256dpi]

Thanks for the contributions! I merged them and just released v0.14.4.

[256dpi]

Thanks for the contribution! But, could you elaborate on how the current behaviour does not match the spec? AFAIK for QoS=2 there are two methods, either yield the message on publish or yield it on pubrel.

[SimonT90poe]

I believe both your existing code and my patch are consistent with the specification. I was referring to the documentation of the MessageCallback hook.

gomqtt/client/service.go

Line 55 in c3c50b8

MessageCallback func(*packet.Message) error

The existing release of this package will announce the message only after completing the QOS2 sequence (on pubrel) and as a result returning an error will cause the connection to drop but the the server will not attempt to redeliver the message - the server had completed its role handling the message.

I can understand that users of the message callback could want either behaviour hence my proposal to add a config option to switch behaviour. This would be something like 'AlwaysAnnounceOnPublish' (default false) thus keeping existing behaviour, while true would cause this new behaviour to come into force.

With regard to the specification you reference it would appear that the authors had experienced similar. Since figure 4.3 references the two delivery options for handling message arrival perhaps it really is worth exposing a config switch and let the package user choose at runtime.

I've tested this patch against mosquitto, vernemq and NATS with MQTT and they all attempt to redeliver the message when the connection resumes. This is ideal behaviour for services consuming MQTT and forwarding into other parts of a data pipeline. If the downstream service is unavailable or unable to accept the message one doesn't need to implement another persistence layer and message ordering guarantees. One just relies on the broker to re-deliver later.

[256dpi]

The existing release of this package will announce the message only after completing the QOS2 sequence (on pubrel) and as a result returning an error will cause the connection to drop but the the server will not attempt to redeliver the message - the server had completed its role handling the message.

The server should attempt to redeliver the Pubrel message if a Pubcomp as never been received. If the callback returns with an error a Pubcomp will never be sent by the client. For verification that the current implementation works I added a test case here: d0c3809.

AFAIK both QoS2 methods are valid, and there should not be a difference wether the message is yield directly on receive or on Pubrel as long as the used sessions store is stable. I opted to chose the current method as the Pubrel message indicates the release and seems more correct.

[SimonT90poe]

I've split the two sets of changes into distinct PRs, #25 and #26. Both now have tests. I was having problems with go test ./... until I found the test environment setup in the github action workflow.

A thing to note: In #26 the function TestServicePublishSubscribeEarlyCallback has a sleep in it. This was the only way I could find to ensure the client would send both pubcomp and disconnect every time. This did bring the test into stability however, I mention this because this might be a hint that not all the client's packets are flushed to the broker before disconnect, or I missed a trick when setting up the test.

PR #25 simply ensures that the online callout will always occur before any message callouts since brokers can start sending messages from persistent connections right after they send their connAck.