Check if channel receive will succeed using len instead of a select statement.#292
Check if channel receive will succeed using len instead of a select statement.#292cdillond wants to merge 2 commits intomicrosoft:mainfrom
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
dlevy-msft-sql
added
bug
| select { | ||
| case tok, more := <-t.tokChan: | ||
| err, more := tok.(error) | ||
| if more { | ||
| t.sess.LogF(t.ctx, msdsn.LogDebug, "nextToken returned an error:"+err.Error()) | ||
| // this is an error and not a token | ||
| return nil, err | ||
| err, ok := tok.(error) | ||
| if ok { | ||
| t.sess.LogF(t.ctx, msdsn.LogDebug, "nextToken returned an error:"+err.Error()) | ||
| // this is an error and not a token | ||
| return nil, err | ||
| } else { | ||
| return tok, nil | ||
| } | ||
| } else { | ||
| return tok, nil | ||
| return nil, nil | ||
| } | ||
| default: | ||
| // there are no tokens on the channel, will need to wait | ||
| } |
There was a problem hiding this comment.
The PR description states that the code was changed to use len() to check the channel length before receiving, but the actual changes still use a select statement with a default case. The PR description does not match the implementation. The actual fix addresses the variable shadowing bug where 'more' was being overwritten by the type assertion result, but does not implement the len()-based approach described.
| return tok, nil | ||
| return nil, nil | ||
| } | ||
| default: |
There was a problem hiding this comment.
The comment explaining the default case behavior was removed. The original comment "there are no tokens on the channel, will need to wait" accurately describes what happens when the default case executes (the code falls through to the blocking select below). Consider restoring this comment for clarity.
| default: | |
| default: | |
| // there are no tokens on the channel, will need to wait |
3 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.