fix: Improve handling of futures and threads during refresh. #1573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
02a439b
to
3d9a19a
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
from
8221fb6
to
3d72001
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
3d9a19a
to
8c6d6f0
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
from
0c52db4
to
f38e968
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
8c6d6f0
to
d3bfaf0
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
from
f38e968
to
4df326f
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
d3bfaf0
to
62a72ad
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
from
4df326f
to
554ee9f
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
62a72ad
to
79378f0
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
from
554ee9f
to
dc0e382
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
79378f0
to
48ae8d1
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
from
dc0e382
to
90f8546
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
2 times, most recently
from
50a8be0
to
4499923
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
from
90f8546
to
9c37eb4
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
4499923
to
f16917f
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
2 times, most recently
from
b064bd6
to
1002788
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
f16917f
to
b14179d
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
b14179d
to
e760519
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
e760519
to
3b68f1f
Compare
hessjcg
force-pushed
the
fix-refresh-timeouts
branch
from
1c70685
to
e1754cc
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
2 times, most recently
from
bd6d635
to
c6f81f9
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
c6f81f9
to
4ad8bbe
Compare
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
4ad8bbe
to
de5b290
Compare
enocom
requested changes
Oct 9, 2023
| // If the currentInstanceData has expired, then force refresh (which will balk if a refresh | ||
| // is already running) and make this and future requests to getInstanceData wait on the | ||
| // refresh operation to complete. | ||
| if (instanceDataFuture.isDone()) { |
There was a problem hiding this comment.
Wouldn't this break our recent ZDT changes, though?
| // If the currentInstanceData has expired, then force refresh (which will balk if a refresh | ||
| // is already running) and make this and future requests to getInstanceData wait on the | ||
| // refresh operation to complete. | ||
| if (instanceDataFuture.isDone()) { |
There was a problem hiding this comment.
If we still think this is important, let's pull it out into a separate PR. It seems like a separate concern from the bigger threading fixes here.
hessjcg
force-pushed
the
fix-refresh-futures
branch
2 times, most recently
from
51a5650
to
d6ccb7e
Compare
|
New PR #1600 makes sure that |
enocom
reviewed
Oct 9, 2023
|
|
||
| AtomicInteger refreshCount = new AtomicInteger(); | ||
| final PauseCondition badRequest1 = new PauseCondition(); | ||
| final PauseCondition badRequest2 = new PauseCondition(); |
There was a problem hiding this comment.
Why two bad requests? Could we write this test with just one?
There was a problem hiding this comment.
Because I want to ensure that the retry is working - that the chain of futures is being built correctly even after the first failure. Early iterations of this PR had a bug where it would retry after one failure, but stop after the second failure.
| badRequest2.proceed(); | ||
| badRequest2.waitForCondition(() -> refreshCount.get() == 3, 2000); | ||
|
|
||
| // Allow the third bad request to complete |
hessjcg
force-pushed
the
fix-refresh-futures
branch
from
d6ccb7e
to
9968302
Compare
enocom
approved these changes
Oct 10, 2023
| // Once rate limiter is done, attempt to getInstanceData. | ||
| ListenableFuture<InstanceData> dataFuture = | ||
| Futures.whenAllComplete(rateLimit) | ||
| .callAsync( |
There was a problem hiding this comment.
Open question: why callAsync and not transformAsync?
|
For my own reference, this is a recreation of #1457. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rewrite the performRefresh() as a chain of task futures from the ListeningScheduledExecutorService. Now, tasks
submitted to the ListeningScheduledExecutorService never block on another task submitted to the ListeningScheduledExecutorService.
This should fix a category of bugs that show up in exceptions and logs as "connection timed out" or "refresh failed"
or "bad client certificate". These exceptions can occur when the credentials fail to refresh.
This is the underlying bug: The ListeningScheduledExecutorService gets into a state where all its threads are busy
running tasks, all running tasks are blocked waiting for recently submitted task to complete, and the recently
submitted tasks can't start because there are no available threads in the ListeningScheduledExecutorService.
This changes the behavior of CloudSqlInstance.getInstanceData() and CloudSqlInstance.startRefreshAttempt()
in ways that have a very small possibility of destabilizing customer applications.
In version 1.14.1 and earlier: CloudSqlInstance.getInstanceData() behaved like this: When no refresh attempt is
in progress, returns immediately. Otherwise, blocks application thread until the current refresh attempt finishes.
If the refresh attempt succeeds, this returns the InstanceData. If not, this throws a RuntimeException, while a
new refresh attempt is submitted to the executor in the background.