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

feat: Substitute mutex with atomic operations. #622

Closed
wants to merge 1 commit into from

Conversation

weezel
Copy link
Contributor

@weezel weezel commented Sep 22, 2024

Use atomic operations instead a mutex. This approach avoids locking and increment can be done within a single clock cycle, hence no deadlock nor race condition can occur.

Use atomic operations instead a mutex. This approach avoids locking and
increment can be done within a single clock cycle, hence no deadlock nor
race condition can occur.

Signed-off-by: Ville Valkonen <weezel@users.noreply.github.com>
@weezel
Copy link
Contributor Author

weezel commented Sep 29, 2024

A gentle one week bump

@weezel
Copy link
Contributor Author

weezel commented Oct 6, 2024

A gentle two weeks bump

Copy link
Member

@bashbunni bashbunni left a comment

Choose a reason for hiding this comment

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

Looks good to me! Will get another team member to also confirm before merging :)

@meowgorithm
Copy link
Member

Thanks for your patience with this one @weezel. If we think this makes sense we should probably do this across the board in Bubbles where we employ this pattern (for example, in spinner).

Before we merge this we should also look into why this is failing in CI.

Comment on lines +22 to +23
lastID.Add(1)
return int(lastID.Load())
Copy link
Member

Choose a reason for hiding this comment

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

add returns the new value, so this can be:

Suggested change
lastID.Add(1)
return int(lastID.Load())
return int(lastID.Add(1))

@caarlos0
Copy link
Member

caarlos0 commented Oct 7, 2024

Before we merge this we should also look into why this is failing in CI.

atomic.Int64 is not available in go 1.18... should we bump it?

caarlos0 added a commit that referenced this pull request Oct 7, 2024
closes #622

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
caarlos0 added a commit that referenced this pull request Oct 7, 2024
closes #622

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
@meowgorithm
Copy link
Member

Unless there are any negative implications, let’s maintain 1.18 compatibility.

caarlos0 added a commit that referenced this pull request Oct 7, 2024
closes #622

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Co-authored-by: Ville Valkonen <weezel@users.noreply.github.com>
caarlos0 added a commit that referenced this pull request Oct 7, 2024
closes #622

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Co-authored-by: Ville Valkonen <weezel@users.noreply.github.com>
@caarlos0 caarlos0 closed this in #634 Oct 7, 2024
@meowgorithm
Copy link
Member

@weezel, just a heads up that we merged this in #634 based on your implementation. Thanks for flagging this one, and for the fix ❤️

@weezel
Copy link
Contributor Author

weezel commented Oct 8, 2024

Thanking you 😊👍

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.

4 participants