blocking call in Subscribe() callback

[sahib]

This is more of a question, than an bug report.

---

This line here looks like someone forgot a go:

https://github.com/eclipse/paho.mqtt.golang/blob/master/router.go#L171

On this line the message is handled in a separate go routine:

https://github.com/eclipse/paho.mqtt.golang/blob/master/router.go#L147

---

I noticed this since I had a (by accident) blocking call in a message handler callback. This apparently caused other message handlers to be not called until the blocking call finished. I realize that blocking in a message handler is not the best idea anyways and might be even expected when SetOrderMatter(true) (the default). So the fix is maybe just to remove the weird func() {} closure around in L171.

I use paho.mqtt v1.2.0 together with mosquitto version 1.6.9.

[MattBrittan]

Hi @sahib,

The lack of a go is deliberate (I suspect you may have guessed that); however, as per your comment, there is no need for the first call (using the order they appear in the code!) to be in a closure.

The first call is for when the order is false (SetOrderMatter(false)). In this case we dont care what order incomming messages are processed meaning it is OK to use a go routine for the handler.

The second one is used when messages need to be processed in order (SetOrderMatter(true)) . Running the handler in a go routine could lead to messages being passed to the handler(s) out of order (the order go routines run in is unpredictable).

As you say blocking unnecessarily in any of the call backs is a bad idea (and I include calling publish from a message handler; unless doing so in a go routine). However there are some use cases where it is important that messages are processed in order and because the spec sets out cases where messages must be delivered in order I think the default is sensible.

Matt

[sahib]

Thank you @MattBrittan for the quick clarification.

Easiest fix for me is then to set SetOrderMatter(false), which should be fine for my specific use-case.

As you say blocking unnecessarily in any of the call backs is a bad idea (and I include calling publish from a message handler; unless doing so in a go routine). However there are some use cases where it is important that messages are processed in order and because the spec sets out cases where messages must be delivered in order I think the default is sensible.

May I suggest to leave a note (e.g. in the docs for the Subscribe() call) to emphasize that blocking in message handlers can lead to this kind of behavior? I did that instinctively in most cases, but a clear word of warning might be useful.

[MattBrittan]

May I suggest to leave a note

You may (it's something I have been meaning to do for some time) but please do feel free to submit a pull request yourself (might happen more quickly than waiting for me!).

[sahib]

Sure 😄 I was more afraid that my wording skills are not that good, but I tried: #428