search

eclipse / paho.golang

Go libraries
Other
327 stars 92 forks source link

Autopaho managePublishQueue() does not call Remove, Quarantine, or Leave if Reader() fails #233

Closed vishnureddy17 closed 6 months ago

vishnureddy17 commented 7 months ago

In autopaho, if entry.Reader() fails we do not call Remove, Quarantine, or Leave on the entry as required. I'm guessing there should be a call to Leave() in the case where Reader() fails. Happy to open a PR for this. https://github.com/eclipse/paho.golang/blob/ef0065fea247d5fb388fa82390cedce694e844e1/autopaho/auto.go#L553-L562

MattBrittan commented 6 months ago

Well spotted; happy to accept a PR. I'm not not sure if it's best to leave or Remove in this case; if we cannot read the message then what are the chances that this would succeed if we try again.

vishnureddy17 commented 6 months ago

if we cannot read the message then what are the chances that this would succeed if we try again.

This is the case that prompted me to open #234

MattBrittan commented 6 months ago

Just noticed a further bug here; we check if errors.Is(err, queue.ErrEmpty) { but ignore other errors (not sure what the right thing to do is; perhaps a small timeout then loop).

vishnureddy17 commented 6 months ago

(not sure what the right thing to do is; perhaps a small timeout then loop).

That seems reasonable. I also think we should have that same timeout in the default case if PublishWithOptions fails with an error that is not paho.ErrNetworkErrorAfterStored or paho.ErrInvalidArguments. Right now, we immediately loop without a delay: https://github.com/eclipse/paho.golang/blob/ef0065fea247d5fb388fa82390cedce694e844e1/autopaho/auto.go#L612-L621