Wiring up Persistence and looking at conn loss #27
Conversation
paho/client.go
Outdated
| @@ -626,6 +652,10 @@ func (c *Client) publishQoS12(ctx context.Context, pb *packets.Publish) (*Publis | |||
| cpCtx := &CPContext{pubCtx, make(chan packets.ControlPacket, 1)} | |||
|
|
|||
| pb.PacketID = c.MIDs.Request(cpCtx) | |||
| if err := c.ClientConfig.Persistence.Put(pb.PacketID, pb); err != nil { | |||
There was a problem hiding this comment.
I wonder if this should be above the c.serverInflight.Acquire?
My view is that any message passed to Publish should be delivered if at all possible (and this includes cases when the connection has dropped or not yet been established). With this code as it currently stands if the connection drops and we then publish enough messages to overwhelm serverInflight any following messages will not be stored (an error will be returned when the context expires).
This does have a couple of potentially negative outcomes:
- If 65534 messages are stored we will run out of IDs (and I think returning an error in that case is fine!)
- The user needs to understand that receiving an error from
Publishdoes not mean that their message will not be delivered (but this is already the case with the timeouts).
paho/client.go
Outdated
| cpCtx.Return <- *recv | ||
| } else { | ||
| pl := packets.Pubrel{ | ||
| PacketID: pr.PacketID, | ||
| } | ||
| //TODO: what to do about failing to persist a pubrel? |
There was a problem hiding this comment.
I don't think there is anything you can do... Seems fairly unlikely and I guess the worst that could happen is that the PUBLISH is resent?
paho/client.go
Outdated
| c.mu.Unlock() | ||
| if c.ClientConfig.OnConnectionLost != nil { |
There was a problem hiding this comment.
I wonder if this (and the calls to Stop) should be within the default select option? I believe that Error may be called multiple times (incomming will call it and the ping routine may also do so). As mentioned in #26 I would also be keen to see a failure to transmit a message to be seen as an error (I know that ping will pick this up eventually but I'm not sure what a user can do about a failew write to the link other than disconnect and try again).
paho/client.go
Outdated
| @@ -145,10 +151,16 @@ func (c *Client) Connect(ctx context.Context, cp *Connect) (*Connack, error) { | |||
| if c.Conn == nil { | |||
| return nil, fmt.Errorf("client connection is nil") | |||
| } | |||
| if err := c.ClientConfig.Persistence.Open(); err != nil { | |||
There was a problem hiding this comment.
Is c.ClientConfig.Persistence.Close() ever called? My feeling is that it should be called in Disconnect() (or a new func (c *Client) Close()) because the user should always call this when exiting (some of this comes back to how the store works before the connection is established and when the connection has dropped). As per #26 I would quite like to see the Open() happen in NewClient() so that it is possible to publish messages before the connection comes up (avoiding the need for the user to create their own store in most cases).
There was a problem hiding this comment.
Looking at this further I could not find the code to send the messages stored upon reconnection; so assume this patch is still a work in progress (please feel free to ignore my comments!).
paho/client.go
Outdated
| OnDisconnect func(*Disconnect) | ||
| ClientID string | ||
| Conn net.Conn | ||
| MIDs MIDService |
There was a problem hiding this comment.
Note that I submitted a pull request to the v3 library that changes the way MIDs are allocated. I believe that this makes debugging/tracing easier because the IDs are not re-used immediatly as they are with the current implementation (it also fixed the issue I had publishing to Mosquitto which I was trying to diagnose at the time)
|
yeah, it's not done yet, thanks for the review I'll take a look |
I've removed the OnDisconnect callback and moved to OnConnectionLost
instead and used a specific Error type to indicate when it was a server
initiated disconnect (see test change)
#25
#26