Check if channel receive will succeed using len instead of a select statement. #292
Conversation
…tended. In its current state, it immediately overwrites the "more" variable received from the tokChan with the result of the type assertion. That seems like a bug. Additionally, the entire pattern that it uses is unnecessary and error prone: select statements are non-deterministic, so the default statement block might execute even if the channel is closed or not empty. I changed the code so that it checks the length of the channel before receiving. This way, the receive is guaranteed not to block and it will always execute whenever the channel is not empty.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #292 +/- ## ========================================== - Coverage 75.32% 75.15% -0.17% ========================================== Files 33 33 Lines 6500 6504 +4 ========================================== - Hits 4896 4888 -8 - Misses 1321 1330 +9 - Partials 283 286 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the token processing logic and improves the channel receive pattern. The existing code had a variable shadowing issue where the more variable from the channel receive was immediately overwritten by the type assertion result, and used a potentially non-deterministic select statement pattern.
- Fixed variable shadowing bug in token channel processing
- Replaced select-with-default pattern with length check for deterministic behavior
- Simplified the non-blocking channel receive logic
token.go Outdated
| case tok, more := <-t.tokChan: | ||
| err, more := tok.(error) | ||
| if more { | ||
| if len(t.tokChan) > 0 { |
There was a problem hiding this comment.
Using len() to check if a channel receive will succeed is not reliable in concurrent code. The channel length can change between the len() check and the subsequent receive operation if another goroutine receives from the channel. This creates a race condition that could cause the goroutine to block indefinitely. The original select statement with default case was the correct approach for non-blocking receives, though the variable naming issue should be fixed differently.
There was a problem hiding this comment.
Oops - I guess I was wrong about how default cases in select statements work. Sorry!
2 participants
I noticed that the existing code probably didn't do quite what was intended. In its current state, it immediately overwrites the "more" variable received from the tokChan with the result of the type assertion. That seems like a bug. Additionally, the entire pattern that it uses is unnecessary and error prone: select statements are non-deterministic, so the default statement block might execute even if the channel is closed or not empty. I changed the code so that it checks the length of the channel before receiving. This way, the receive is guaranteed not to block and it will always execute whenever the channel is not empty.