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: data race in Default and SetDefault #122

Merged
merged 2 commits into from
May 7, 2024

Conversation

op
Copy link
Contributor

@op op commented May 3, 2024

This adds synchronisation primitives around all uses of the defaultLogger.

atomic.Pointer was introduced in Go 1.19. I see you have 1.19 in .github/ and go.mod, but maybe you still try to be backwards compatible? If so, I can replace atomic.Pointer with atomic.Value.

UPDATE: I see now that in #111, on purpose we delay initialisation of the logger. Reading about init in Effective Go it looks like using func init() should work fine.

[..] init is called after all the variable declarations in the package have evaluated their initializers, and those are evaluated only after all the imported packages have been initialized.

(An alternative solution exists at e31f99c but is more complicated imho. Most likely the problem with escape sequence during initialisation was because of the race within this package.)

Fixes #121.

@op op requested a review from aymanbagabas as a code owner May 3, 2024 08:19
@op op changed the title Fix data race in Default and SetDefault fix: data race in Default and SetDefault May 3, 2024
@op op force-pushed the op/default-race branch 2 times, most recently from e31f99c to 3e0b742 Compare May 3, 2024 11:25
Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.03%. Comparing base (2338a13) to head (3e0b742).
Report is 15 commits behind head on main.

❗ Current head 3e0b742 differs from pull request most recent head e31f99c. Consider uploading reports for the commit e31f99c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
+ Coverage   80.92%   81.03%   +0.11%     
==========================================
  Files          11       11              
  Lines         739      654      -85     
==========================================
- Hits          598      530      -68     
+ Misses        126      109      -17     
  Partials       15       15              

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

@aymanbagabas
Copy link
Member

Thank you @op for the PR!

atomic.Pointer was introduced in Go 1.19. I see you have 1.19 in .github/ and go.mod, but maybe you still try to be backwards compatible? If so, I can replace atomic.Pointer with atomic.Value.

Go 1.19 looks good, this is the minimum Go requirement for this package.

UPDATE: I see now that in #111, on purpose we delay initialisation of the logger. Reading about init in Effective Go it looks like using func init() should work fine.

[..] init is called after all the variable declarations in the package have evaluated their initializers, and those are evaluated only after all the imported packages have been initialized.

(An alternative solution exists at e31f99c but is more complicated imho. Most likely the problem with escape sequence during initialisation was because of the race within this package.)

I'm leaning more towards the alternative solution since AFAICT, in some cases init() can get called before Default(). We want to delay the logger initialization till the first call to log.

@op
Copy link
Contributor Author

op commented May 7, 2024

@aymanbagabas Sounds fair! I've updated the PR to make sure that you're able to cal SetDefault to prevent any default initialisation taking place.

@aymanbagabas aymanbagabas merged commit deae1b0 into charmbracelet:main May 7, 2024
24 checks passed
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.

Data race in Default and SetDefault
2 participants