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

fix(store/odsq4): fix order of opened flag stored #3735

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

walldiss
Copy link
Member

Opened bool was stored before actual value of q4 was updated, which made it possible for concurrent reader to read nil value before it was updated.

Alternatively code can be refactored to use RWMutex instead of Mutex+atomic.Bool, which can improve readability of logic.

@cristaloleg
Copy link
Contributor

Sounds like sync.Once to me. However it's up to you.

Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

SGTM

@walldiss
Copy link
Member Author

We need to store state here for stored q4 to be able to know if we need to close it or not. sync.Once doesn't allow to read if it was executed or not.

@walldiss walldiss added shwap kind:fix Attached to bug-fixing PRs labels Sep 17, 2024
@Wondertan
Copy link
Member

@cristaloleg, see this golang/go#41690

@cristaloleg
Copy link
Contributor

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

good catch!

store/file/ods_q4.go Show resolved Hide resolved
store/file/ods_q4.go Show resolved Hide resolved
@Wondertan
Copy link
Member

@cristaloleg, this does not work for this case we need, unfortunately. We need to know if the once actually happened, while the OnceValue only gives a memoized value, which might be zero or not on its own

@walldiss walldiss merged commit e6d3a58 into celestiaorg:shwap Sep 17, 2024
15 of 16 checks passed
@walldiss walldiss deleted the odsq4-fix-open-order branch September 17, 2024 14:46
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (shwap@15ebe17). Learn more about missing BASE report.

Files with missing lines Patch % Lines
store/file/ods_q4.go 60.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             shwap    #3735   +/-   ##
========================================
  Coverage         ?   46.68%           
========================================
  Files            ?      314           
  Lines            ?    17820           
  Branches         ?        0           
========================================
  Hits             ?     8319           
  Misses           ?     8490           
  Partials         ?     1011           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:fix Attached to bug-fixing PRs shwap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants