Skip to content

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

Merged
QuintinWillison merged 5 commits into
mainfrom
fix_waiter_close_issue
Jul 19, 2022
Merged

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

Conversation

@ikbalkaya

@ikbalkayaikbalkaya commented Jul 15, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes bug defined #823

Closes#823

@ikbalkaya
ikbalkaya marked this pull request as draft July 15, 2022 13:53
@ikbalkayaikbalkaya changed the title Moved waiter.close inside background thread after while breakcall waiter.close() after while loop breaksJul 15, 2022
@ikbalkayaikbalkaya changed the title call waiter.close() after while loop breakscall waiter.close() after breaking from while loopJul 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

@QuintinWillisonQuintinWillison left a comment

Copy link
Copy Markdown
Contributor

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

ikbalkaya commented Jul 15, 2022

Copy link
Copy Markdown
ContributorAuthor

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
Comment threadlib/src/main/java/io/ably/lib/transport/ConnectionManager.java Outdated
@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
@ikbalkayaikbalkaya self-assigned this Jul 18, 2022
@ikbalkayaikbalkaya 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

@KacperKlukaKacperKluka left a comment

Copy link
Copy Markdown
Contributor

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

@QuintinWillisonQuintinWillison left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadlib/src/test/java/io/ably/lib/test/realtime/RealtimeAuthTest.java
qsdigor
qsdigor approved these changes Jul 19, 2022
View reviewed changes

@qsdigorqsdigor left a comment

Copy link
Copy Markdown
Contributor

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 mainJul 19, 2022
@QuintinWillison
QuintinWillison deleted the fix_waiter_close_issue branch July 19, 2022 11:56
@QuintinWillisonQuintinWillison mentioned this pull request Nov 23, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@QuintinWillisonQuintinWillisonQuintinWillison approved these changes
@qsdigorqsdigorqsdigor approved these changes
@KacperKlukaKacperKlukaKacperKluka approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ikbalkayaikbalkaya

Labels

bugSomething 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