Add hooks for onUnsubscribe, before unsubscribe happens. #26
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
==========================================
+ Coverage 70.25% 70.33% +0.07%
==========================================
Files 26 26
Lines 2700 2353 -347
==========================================
- Hits 1897 1655 -242
+ Misses 639 541 -98
+ Partials 164 157 -7
Continue to review full report at Codecov.
|
|
merged. Nice feature, thanks for your PR. |
| @@ -667,6 +667,9 @@ func (client *client) unsubscribeHandler(unSub *packets.Unsubscribe) { | |||
| unSuback := unSub.NewUnSubBack() | |||
| client.write(unSuback) | |||
| for _, topicName := range unSub.Topics { | |||
| if srv.hooks.OnUnsubscribe != nil { | |||
| srv.hooks.OnUnsubscribe(context.Background(), client, topicName) | |||
There was a problem hiding this comment.
Question here, what will you do if the topic is invalid? It seams the topic will still be unsubscribed.
There was a problem hiding this comment.
I was using gmqttd as a proxy, because the hooks were very helpful. No, it doesn't help that there's no return type nor error condition, because all that stuff is in the hook (rest of the processing is ignored completely)
However, it makes sense to use return types such as nil / Error in addition to current values, even for the onSubscribe sort of hooks, because an error processing can allow us to prevent further processing, than just qos settings. I wanted this to currently be just that helpful lifecycle handler for now, but I do agree that there is more logic that could be added here.
The current answer may be to panic/recover but I would highly advise against that sort of code. :)
There was a problem hiding this comment.
However, it makes sense to use return types such as nil / Error in addition to current values, even for the onSubscribe sort of hooks, because an error processing can allow us to prevent further processing, than just qos settings
I agree. Because the v3.1.1 specification does not define "how to response the client when error happens", gmqtt cannot decide what to do when receiving an error from hooks. But you can close the client in the hook, which is the general solution for MQTT v3/v3.1.1.
MQTT v5 gives every acknowledgement packet a reason code and reason string which can use to inform the client when error happens, gmqtt will also add error return for most hooks to support such feature.
There was a problem hiding this comment.
Thank you, and looking forward to that :-) May I request that you make an interim release (v0.1.3) if possible?
|
@guilt no problem, done. |
I have created this pull request because I had an issue where I had to check if channel was valid/met some constraints, before unsubscribing from it, and cannot do this with
onUnsubscribed(). Hence this PR.Please see if you can get this behavior merged, would be very helpful. I currently maintain a vendored fork because of this.