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: make timer operations work with golang 1.23 #667

Closed
wants to merge 5 commits into from

Conversation

gammazero
Copy link
Contributor

In golang 1.23 reading a timer's channel after calling timer.Stop will block forever. This is fixed by looking at the timer's channel capacity to determine if the channel may need to be read following a Stop.

In golang 1.23 reading a timer's channel after calling timer.Stop will block forever. This is fixed by looking at the timer's channel capacity to determine if the channel may need to be read following a Stop.
@gammazero gammazero requested review from lidel and a team as code owners September 5, 2024 16:41
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 60.20%. Comparing base (51da02f) to head (789ea52).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
mfs/repub.go 84.61% 2 Missing ⚠️
gateway/backend_car_traversal.go 0.00% 0 Missing and 1 partial ⚠️
provider/reprovider.go 0.00% 0 Missing and 1 partial ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #667      +/-   ##
==========================================
+ Coverage   60.18%   60.20%   +0.01%     
==========================================
  Files         241      241              
  Lines       30707    30707              
==========================================
+ Hits        18482    18486       +4     
+ Misses      10575    10570       -5     
- Partials     1650     1651       +1     
Files with missing lines Coverage Δ
gateway/backend_car_traversal.go 72.72% <0.00%> (ø)
provider/reprovider.go 68.16% <0.00%> (ø)
mfs/repub.go 80.61% <84.61%> (+1.02%) ⬆️

... and 13 files with indirect coverage changes

@Stebalien
Copy link
Member

Note: it looks like go is fixing this upstream, so it might not be worth it to add this workaround.

@gammazero
Copy link
Contributor Author

Even so, clearing the channel is not necessary. The timeout of 1 hour immediately followed by Stop means the timer will never fire, so no need to read from the channel.

@Stebalien
Copy link
Member

Even so, clearing the channel is not necessary. The timeout of 1 hour immediately followed by Stop means the timer will never fire, so no need to read from the channel.

Yeah, but I prefer to make code timing independent. Any reason not to just wait for 1.24, then bump the minimum go requirement to 1.23 to completely fix the race?

@gammazero
Copy link
Contributor Author

@Stebalien I made the code time independent. This PR is still needed since there are places where the timer channel is read following a Stop, and this breaks with go1.23.

@Stebalien
Copy link
Member

@gammazero golang/go#69312? My understanding is that this is going to be fixed in a patch release very soon.

@gammazero
Copy link
Contributor Author

Will wait for go fix. Until then, the problem can be avoided by running with GODEBUG="asynctimerchan=1"

@gammazero gammazero closed this Sep 19, 2024
@gammazero gammazero deleted the fix/golang1.23-timers branch September 19, 2024 15:54
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.

2 participants