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

Replaced topic lock by atomic boolean to avoid lock contention #495

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aratz-lasa
Copy link

PR opened in response to #494.

In the topic struct, there is currently lock contention due to the internal lock that is used to check if the topic has been closed or not. This generates lock contention, in those cases where the publish call takes a long time, such as if messages are signed with RSA.

Therefore, in this PR is proposed to replace the lock with an atomic boolean, allowing concurrent calls to Publish, and relieving lock contention.

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not entirely the same, but i think this is fine.
I am a little surprised there is contention with the RW lock.

Can you gofmt?

topic.go Show resolved Hide resolved
@aratz-lasa
Copy link
Author

I ran gofmt to all the source code and pushed again.

Lock contention occurs because the pubsub producers go faster than the time it takes to sign with Ed25519, therefore, they end up queueing up, since they cannot enter the publish call until the lock is free.

What do you mean by "it's not entirely the same"?

@vyzo
Copy link
Collaborator

vyzo commented Jul 22, 2022

its an RLock, so they should not content, they can hold it concurrently.
Only close takes the exclusive lock.

It is not entirely the same because an active publish would prevent the topic from being closed concurrently.

@vyzo
Copy link
Collaborator

vyzo commented Jul 22, 2022

Let me think about it a bit.

I dont think there will be any deleterious effects from the change, but it is still a change in behaviour.

@aratz-lasa
Copy link
Author

aratz-lasa commented Jul 22, 2022

Aaah you are right! I thought that the RLock could only be acquired by goroutines in the same thread (since that's how it works in other languages). However I looked it up and I was wrong. There can be concurrent publish calls despite the goroutines being in different threads. So it's your call!

t.mux.Lock()
defer t.mux.Unlock()
if t.closed {
if t.closed.Load() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a race here: you want to do Swap here and if old result was false then proceed with closing.

Otherwise 2 goroutines can hit Close, load false and proceed.

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

Successfully merging this pull request may close these issues.

3 participants