Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace mutex in autopaho with atomic pointer #279

Open
thedevop opened this issue Nov 24, 2024 · 4 comments
Open

Replace mutex in autopaho with atomic pointer #279

thedevop opened this issue Nov 24, 2024 · 4 comments

Comments

@thedevop
Copy link
Contributor

thedevop commented Nov 24, 2024

Is your feature request related to a problem? Please describe.
Autopaho currently use mutex for Publish/Subscribe/etc. It can lead to lock contentions.

Describe the solution you'd like
Replace mutex with atomic.Pointer. Put cli, connUp, and connDown into one struct, so it can be stored in one atomic operation.

Additional context
Here is the implementation, only autopaho/auto* are modified.

@MattBrittan
Copy link
Contributor

Can you please provide more information re "lock contentions", have you identified a performance issue in a real application which this change fixes?

I'm open to the use of atomic.Pointer but feel that mutex is generally easier to understand and more widely used. As such I have a preference for a mutex unless there is a fairly compelling reason to change (in line with the docs "Except for special, low-level applications, synchronization is better done with channels or the facilities of the sync package").

@thedevop
Copy link
Contributor Author

thedevop commented Nov 24, 2024

The lock contentions stem from upper layer application, where there are many Go routines that Publish/Subscribe/Unsubscribe through the client. But with current Subscribe/Unsubscribe (and to certain extent Publish, as async Publish doesn't return broker ack) as blocking operations until ack from broker, funneling them through a single channel is not viable.

@MattBrittan
Copy link
Contributor

And has the change to atomic.Pointer made a measurable difference? The locks on ConnectionManager.Mu should be very brief because it's only locked for the time taken to grab a copy of cli. async Publish is the subject of issue #216 and this is complicated by the queue and store (I have made a suggestion as to one way to address this which would be fairly easy to implement but looking for some feedback).

@thedevop
Copy link
Contributor Author

thedevop commented Dec 8, 2024

In grand scheme of things, this probably will not make large differences. I agree with you that using low-level atomics can be harder to understand and easier to make mistakes. However, for this use case, it can make the code cleaner and easier to understand.

Currently there is cli (client) with 2 states connUp and connDown. Using atomic.Pointer allows these 2 states (actually they're more gates) to be associated with the client as one unit:

  1. In initial state, it's in a DownState: cli=nil, connDown channel is closed
  2. Once a client makes a connection, the state atomically swaps DownState to UpState, with cli=new client, connUp channel is closed. It also close the previous DownState connDown channel.
  3. If client connection is broken, the state atomically swaps UpState to Downstate (with value as in 1), and closes the previous UpState connDown channel.
  4. Repeat step 2 and 3 as client connection is established and broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants