Skip to content

call waiter.close() after breaking from while loop#825

Merged
QuintinWillison merged 5 commits intomainfrom
fix_waiter_close_issue
Jul 19, 2022
Merged

call waiter.close() after breaking from while loop#825
QuintinWillison merged 5 commits intomainfrom
fix_waiter_close_issue

Conversation

@ikbalkaya
Copy link
Contributor

@ikbalkaya ikbalkaya commented Jul 15, 2022 •
edited
Loading

Fixes bug defined #823

Closes #823

@ikbalkaya ikbalkaya marked this pull request as draft July 15, 2022 13:53
@ikbalkaya ikbalkaya changed the title Moved waiter.close inside background thread after while break call waiter.close() after while loop breaks Jul 15, 2022
@ikbalkaya ikbalkaya changed the title call waiter.close() after while loop breaks call waiter.close() after breaking from while loop Jul 15, 2022
@ikbalkaya ikbalkaya marked this pull request as ready for review July 15, 2022 14:15
QuintinWillison
QuintinWillison reviewed Jul 15, 2022
View reviewed changes
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Is this something we can add a test for?

@ikbalkaya
Copy link
Contributor Author

ikbalkaya commented Jul 15, 2022

Is this something we can add a test for?

@QuintinWillison In the change I made, there was a test for old renew() that I wrote the equivalent for for the new method. However I can see that we need a test specifically to test that we can get a result from this renewal method. I will add at least a test to confirm that we are getting a result back

KacperKluka
KacperKluka reviewed Jul 15, 2022
View reviewed changes
@ikbalkaya ikbalkaya marked this pull request as draft July 15, 2022 15:20
@ikbalkaya ikbalkaya marked this pull request as draft July 15, 2022 15:20
@ikbalkaya ikbalkaya self-assigned this Jul 18, 2022
@ikbalkaya ikbalkaya added the bug Something isn't working. It's clear that this does need to be fixed. label Jul 18, 2022
@ikbalkaya ikbalkaya marked this pull request as ready for review July 19, 2022 08:50
KacperKluka
KacperKluka approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@KacperKluka KacperKluka left a comment

Choose a reason for hiding this comment

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

LGTM 👍

QuintinWillison
QuintinWillison approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

One minor observation, but happy to land this without fixing it up, as I know we're keen to get a release out that includes this fix.

qsdigor
qsdigor approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@qsdigor qsdigor left a comment

Choose a reason for hiding this comment

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

Very light review but looks ok.

@QuintinWillison QuintinWillison merged commit 1f378a8 into main Jul 19, 2022
@QuintinWillison QuintinWillison deleted the fix_waiter_close_issue branch July 19, 2022 11:56
@QuintinWillison QuintinWillison mentioned this pull request Nov 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@QuintinWillison QuintinWillison QuintinWillison approved these changes

@qsdigor qsdigor qsdigor approved these changes

@KacperKluka KacperKluka KacperKluka approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ikbalkaya ikbalkaya

Labels

bug Something isn't working. It's clear that this does need to be fixed.

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

waiter.close() is invoked early on onAuthUpdatedAsync method

4 participants

@ikbalkaya @QuintinWillison @qsdigor @KacperKluka