eclipse / paho.golang

Go libraries
Other
327 stars 92 forks source link

feat: Update dependencies and add auto reconnect functionality #208

Closed ZekeButlr closed 9 months ago

ZekeButlr commented 9 months ago

Do not create a Pull Request without creating an issue first

The design of your solution to the problem should be discussed and agreed in an issue before submitting the PR

ECA - Eclipse Contributer Agreement

Ensure you have a valid, signed ECA for the email address associated with your Github account (or you sign off your PR with an ECA'd address)

Please provide enough information so that others can review your pull request:

Testing

Any code changes should have accompanying tests.

Closing issues

Put closes #XXXX in your comment to auto-close the issue that your PR fixes.

MattBrittan commented 9 months ago

As per the template it's suggested that "The design of your solution to the problem should be discussed and agreed in an issue before submitting the PR".

paho was designed to accept a net.Conn and effectively terminate when that connection is lost. Adding automatic reconnection functionality substantially complicates this, and the design of any such solution would need to be carefully considered. For example:

The above is from a quick look; I suspect there will be other issues (because paho has been written with the assumption that it manages a single connection and is then disposed of).

The v3 client performs reconnects in a similar manner to what you are proposing, our experience with that is that it's very difficult to get right (it's taken years to resolve deadlocks/races etc). This is one reason that the currently client is designed in the way it is; having a client whose lifecycle matches the connection (i.e. it's used once) makes it easier to reason about.

Currently automatic reconnection options are provided by autopaho (which also offers functionality like queueing messages while the client is connecting/reconnecting).

As a result of the above I feel that it's unlikely that this PR will be accepted; happy to discuss further if you create an issue (it would be useful to know why you prefer this approach over the existing one).

ZekeButlr commented 9 months ago

As per the template it's suggested that "The design of your solution to the problem should be discussed and agreed in an issue before submitting the PR".

paho was designed to accept a net.Conn and effectively terminate when that connection is lost. Adding automatic reconnection functionality substantially complicates this, and the design of any such solution would need to be carefully considered. For example:

  • Avoid data races (in the PR there are races on c.Conn and c.AutoReconnectConfig.subscription because users may still be using the library i.e. calling Subscribe etc)
  • Consider how reconnections interact with calls to, for instance, Publish (when the connection is down `Publish would return an error; this means the users code needs to handle such errors and, potentially, resend the message when the connection comes up).
  • Provide a way to stop operations (calling client.Disconnect() would trigger your reconnection code which is somewhat confusing).
  • Manage things like the store (this may be closed in close() and your code does not recreate/reopen it)

The above is from a quick look; I suspect there will be other issues (because paho has been written with the assumption that it manages a single connection and is then disposed of).

The v3 client performs reconnects in a similar manner to what you are proposing, our experience with that is that it's very difficult to get right (it's taken years to resolve deadlocks/races etc). This is one reason that the currently client is designed in the way it is; having a client whose lifecycle matches the connection (i.e. it's used once) makes it easier to reason about.

Currently automatic reconnection options are provided by autopaho (which also offers functionality like queueing messages while the client is connecting/reconnecting).

As a result of the above I feel that it's unlikely that this PR will be accepted; happy to discuss further if you create an issue (it would be useful to know why you prefer this approach over the existing one).

Yea can we move this discussion to an issue then? I'm okay closing this PR. The issue is.... reconnect is then pushed off to the consumer of your library... which unfortunately they will never get right. We have about 7 project using various techniques to reconnect and non of the projects have gotten it right.