API Redesign Thoughts

[shoenig]

Hi Mike and Allan,

So I like the idea of moving towards using channels in our API, but I don't think what we have right now quite make sense.

My concern is that we allow setting an OnPublishReceived callback containing a channel on which all messages will be received. From my understanding, the new Router piece will limit the messages arriving on that channel to be only those from topics on which the subscription is based. However, I don't see the use of a channel here as a good idea; the only thing a client app can do with it is receive from that channel over and over. If they do that in multiple instances of the callback (like, in a for {} loop), they'll be receiving duplicate messages, since messages put onto channels are received by ALL of the listeners. If we must stick with the callback paradigm, it would make more sense to only have a Message as an argument, not a channel of all the messages.

However, I think there is a better design out there - trading callbacks for channels.

Instead of registering any sort of OnPublishReceived callback, how about we take advantage of the Router idea, and have StartSubscription return a read-only channel of type Message which can then be used by the client to listen for messages in whatever fashion they wish. Because we will be able to route messages, only messages which originate from the topics of that subscription will arrive on that channel.

Pushing this idea further, methods Publish and PublishMessage could return a read only channel of type Receipt (or maybe just bool?). When the client confirms the message has been delivered, it can put a message on that channel, allowing the client app to know the message has been published.

Pushing the idea a little further, EndSubscription could close the channels opened by StartSubscription, which seems like a very natural way to notify listeners of those channels that there is nothing else. Disconnect would do similar. Note: the way to implement this would involve creating a separate channel for every individual topic, and then return a channel that receives from those channels to StartSubscription. That way, when you specify specific topics to end via EndSubscription, you close exactly those channels, and then you can still receive messages from channels of remaining topics. OR, you could have StartSubscription just return a list of <-chan Message, one for each topic provided.

I don't think I would make a change to the OnConnectionLost callback... pretty much the only thing anyone would use it for is to call Start again, and already serves that purpose well.

In short, some prototypes for this new API would look something like...

func (c *MqttClient) Start() error // unchanged

func (c *MqttClient) Publish( ... ) <- chan Receipt  // a receipt would arrive when the publish flow is complete

func (c *MqttClient) PublishMessage(...) <- chan Receipt // same as Publish

func (c *MqttClient) StartSubscription(...) <- chan Message // Incoming publish messages of the topics of this subscription arrive on this channel

func (c *MqttClient) EndSubscription(...) // closes the channels of the topics listed (in addition to unsubbing from the server)

Thoughts??

[alsm]

I had intended that each client would have a Router, when you do a StartSubscription you provide a callback for that subscription, StartSubscription would call AddRoute on the Router. When a message arrives RouteAndDispatch looks to see what routes (if any) match the topic on the incoming message and call the appropriate callbacks with a copy of the message. We can still blend this with more channels as per your ideas above. My only concern with channels all the way is that they are potentially blocking, the parts of the code we control we can make sure that no blocking calls are made, but with the user able to register their own handler functions if they don't handle their incoming messages appropriately you could end up with blocking all the way back to core network handlers. Either we use go routines at the edge here so they can't muck anything up, or just accept that such blocking is possible and people will realise when they've done it. I don't have a preference either way tbh, just to make sure we've thought about it.

I definitely like the idea of Publish and PublishMessage returning a channel to indicate when the message has sent.

[shoenig]

Oh I see what you mean now. I like this RouteAndDispatch idea.

[shoenig]

I'm going to backtrack on returning Receipt channels... there's no good way to make this scale. If you have a subscriber ignoring receipts, the client will lock up at 2^16-1 messages. Instead, I think we should just go back to the classic OnMessageDelivered callback.

Update: Just kidding, as Mike pointed out, we can put a value on a channel, close that channel, and then still receive that value, which is good enough.