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

[27.x backport] oauth/api: drain timer channel on each iteration #5399

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

vvoland
Copy link
Collaborator

@vvoland vvoland commented Sep 3, 2024

Fixes #5375

- What I did

Previously, if while polling for oauth device-code login results a user suspended the process (such as with CTRL-Z) and then restored it with fg, an error might occur in the form of:

failed waiting for authentication: You are polling faster than the specified interval of 5 seconds.

This is due to our use of a time.Ticker here - if no receiver drains the ticker channel (and timers/tickers use a buffered channel behind the scenes), more than one tick will pile up, causing the program to "tick" twice, in fast succession, after it is resumed.

- How I did it

The new implementation replaces the time.Ticker with a time.Timer (time.Ticker is just a nice wrapper) and introduces a helper function resetTimer to ensure that before every select, the timer is stopped and it's channel is drained.

- How to verify it

Reproduce as described in #5375

- Description for the changelog

Fix issue that will sometimes cause the browser-login flow to fail if the CLI process is suspended and then resumed while waiting for the user to authenticate.

- A picture of a cute animal (not mandatory but encouraged)

Previously, if while polling for oauth device-code login results a user
suspended the process (such as with CTRL-Z) and then restored it with
`fg`, an error might occur in the form of:

```
failed waiting for authentication: You are polling faster than the specified interval of 5 seconds.
```

This is due to our use of a `time.Ticker` here - if no receiver drains
the ticker channel (and timers/tickers use a buffered channel behind the
scenes), more than one tick will pile up, causing the program to "tick"
twice, in fast succession, after it is resumed.

The new implementation replaces the `time.Ticker` with a `time.Timer`
(`time.Ticker` is just a nice wrapper) and introduces a helper function
`resetTimer` to ensure that before every `select`, the timer is stopped
and it's channel is drained.

Signed-off-by: Laura Brehm <[email protected]>
(cherry picked from commit 60d0450)
Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland added this to the 27.2.1 milestone Sep 3, 2024
@vvoland vvoland requested a review from laurazard September 3, 2024 11:22
@vvoland vvoland self-assigned this Sep 3, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2024

Codecov Report

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

Project coverage is 61.01%. Comparing base (6274754) to head (1dfd11a).
Report is 5 commits behind head on 27.x.

Additional details and impacted files
@@            Coverage Diff             @@
##             27.x    #5399      +/-   ##
==========================================
+ Coverage   60.99%   61.01%   +0.02%     
==========================================
  Files         304      304              
  Lines       21357    21369      +12     
==========================================
+ Hits        13026    13038      +12     
  Misses       7402     7402              
  Partials      929      929              

@vvoland vvoland requested a review from a team September 3, 2024 11:52
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 5f8416e into docker:27.x Sep 3, 2024
105 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants