Allow initializing MIDs with in-use IDs, remove Clear(), use RWMutex for Get()

[vishnureddy17]

As discussed in #134, we need a way to claim packet IDs as a prerequisite for connecting with existing sessions. This implements that. In addition, I put an RWMutex in since calls to Get() can safely happen in parallel.

[MattBrittan]

paho never reconnects (so, unless this changes, loading MIDs would be done in another package, i.e. AutoPaho). As such, I wonder if adding Get() is the right approach (not criticising your PR, just offering an alternative viewpoint).

An alternative would be to add a NewMids(c []*CPContext) *MIDs (or perhaps NewMids(map[uint16]*CPContext) *MIDs might be simpler to use) that can be called to create a pre-populated MIDs. This would make it clear that MIDs should only be setup before the connection is established and not altered afterwards (personally I'm not sure that Clear() is needed; the difference between Clear and creating a whole new MIDs seems minor).

[vishnureddy17]

Good points, I'll find some time to update this PR.

[vishnureddy17]

PR updated based on the feedback.

As a side note, I think that using names like "message ID" and "MID" should be avoided because the MQTT spec uses "Packet ID" instead.

[vishnureddy17]

@MattBrittan I just realized that this would prevent users from making their own MIDService. Marking a Packet ID as "in use" needs to be done with a method in the MIDService interface. Also, I think claiming Packet IDs should be done right before sending a connect rather than client instantiation, since opening the persistence to get the in-use packet IDs can be blocking. Thoughts?

Do we even need to support custom packet ID allocators? @alsm, did you have a use-case for custom packet ID allocators in mind when you implemented this?

[MattBrittan]

Marking a Packet ID as "in use" needs to be done with a method in the MIDService interface

Sorry - I'm not seeing (possibly missing something!) why this is a requirement? 'paho' only needs access to Request, Get and Free. Someone creating a new implementation would have access to their implementation (i.e. direct access to the struct, or additional functions not included in the interface).

Also, I think claiming Packet IDs should be done right before sending a connect

I guess this is possible in the current design (just load the data the first time one of the functions is called). The alternative would be a callback just before sending connect (somewhere around here); we will need a few more callbacks (or similar) to implement persistent sessions (once the connection is up any unacknowledged messages will need to be resent). However I feel that trying to cater for this now is putting the horse before the cart (we need to work out how session persistence will work and then change paho to suit).

If AutoPaho was to be modified to support a custom MIDService then this may become an issue (because I would expect AutoPaho to provide a store which it would need to retrieve upon connection). However I don't really see any need for AutoPaho to support a custom implementation of MIDService (my aim is for AutoPaho to support most basic use-cases; if someone wants to do something unusual then they can use paho directly).

[vishnureddy17]

Sorry - I'm not seeing (possibly missing something!) why this is a requirement? 'paho' only needs access to Request, Get and Free.

Paho also needs access to some way to mark a packet ID as in-use if it is connecting with an existing session, no? That's why I'm thinking that the marking of Packet IDs as in-use needs to be a part of the MIDService interface (e.g., with a ClaimID method). Paho will need to get the in-use packet IDs from the persistence implementation, and that may require I/O, so I don't think marking Packet IDs as in-use should be done in the initialization.

I guess this is possible in the current design (just load the data the first time one of the functions is called). The alternative would be a callback just before sending connect (somewhere around here); we will need a few more callbacks (or similar) to implement persistent sessions (once the connection is up any unacknowledged messages will need to be resent).

Yep, I think around there sounds right. Before sending the CONNECT packet, Paho will get the session state from the persistence implementation and mark the relevant packet IDs as in-use. When the CONNACK comes back, Paho will do whatever delivery retry it needs to do per the MQTT requirements.

[MattBrittan]

I think a lot of this comes down to a discussion around the desired level of functionality in the paho package. Currently paho accepts a pre-populated net.Conn and runs from there (i.e. sends CONNECT packet, fully terminates when connection is lost). I really like this design because it keeps all of the connection management stuff separate (this caused a lot of issues in the 3.1.1 client due to confusing interactions between the various running processes).

If paho accepts a pre-connected net.Conn (meaning that it cannot reconnect) then I feel that managing persistence of the session state should also be out of scope. Of course paho needs to interact with the state (flagging that a PUBLISH has been sent, the receipt of ACK packets etc) and my current thought is that this could be handled by an extended version of MIDService (do need to do some experimentation to check that this will work).

Doing things this way keeps paho really simple. If someone wants a really basic MQTT client (i.e. for embedded use) then they can just use paho (perhaps using autopaho as documentation on best practice use). Whereas if a user wants a full client then autopaho will offer that (with a small selection of common options), or they can fork autopaho if they want to do something unusual (a big issue with the 3.1.1 client was that it tried to cater for every requirement).

So, with the above in mind, I feel that the task of loading the session state (to pre-populate MIDService) should be managed outside of paho; hence my comments around there being no need for paho to mark of Packet IDs as in-use (because this would also be done externally). This really all comes back to the big question, "how do we handle session persistence", and I'm reluctant to implement functionality that may be needed to support this (because once users have used a feature its difficult to remove/change it later - don't want to break end users code!).

I'll try to put some time over the next couple of weeks into fleshing out how the store might work; perhaps it might be worth having a discussion around this (hopefully you have had a chance to talk to Al?).

[vishnureddy17]

If paho accepts a pre-connected net.Conn (meaning that it cannot reconnect) then I feel that managing persistence of the session state should also be out of scope. Of course paho needs to interact with the state (flagging that a PUBLISH has been sent, the receipt of ACK packets etc)

Good point.

my current thought is that this could be handled by an extended version of MIDService (do need to do some experimentation to check that this will work).

Why MIDService and not the persistence implementation (however that'd look like)? I'm wondering if you have a good mental model for what should belong in the packet ID tracker vs. the persistent session store. It might clarify my thinking.

I'll try to put some time over the next couple of weeks into fleshing out how the store might work; perhaps it might be worth having a discussion around this (hopefully you have had a chance to talk to Al?).

Happy to have a discussion on or off GitHub—you have my e-mail address. Yep, I've talked to Al!

[MattBrittan]

Why MIDService and not the persistence implementation (however that'd look like)?

Just a thought at this point. The lifecycle of a MID matches that of an outgoing message (allocated when message created, released after appropriate acknowledgments received) so it may make sense to combine the two.

[MattBrittan]

I'll close this one off as I believe PR #172 addresses the requirement in a different way (and significantly changes how message IDs are handled meaning this PR is probably no longer relevant).