Fix Timer resource teardown #16289
Merged
Fix Timer resource teardown #16289
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
pbatko-da
approved these changes
Feb 13, 2023
skisel-da
reviewed
Feb 13, 2023
libs-scala/resources/src/main/scala/com/digitalasset/resources/TimerResourceOwner.scala
Show resolved
Hide resolved
pbatko-da
reviewed
Feb 13, 2023
libs-scala/resources/src/main/scala/com/digitalasset/resources/TimerResourceOwner.scala
Show resolved
Hide resolved
After this PR the TimerResourceOwner will make sure that none of the scheduled task are running, after the Resource finished releasing. Also adds asynchronous test as evidence. [CHANGELOG_BEGIN] [CHANGELOG_END]
nmarton-da
force-pushed
the
nmarton/fix-timer-resource-teardown
branch
from
5c593f2
to
a40c59c
Compare
skisel-da
reviewed
Feb 13, 2023
| info("And as waiting 100 millis") | ||
| released.isCompleted shouldBe false | ||
| info("The release should not be completed yet") | ||
| val extraTimerTaskStarted = Promise[Unit]() |
There was a problem hiding this comment.
I think this could be just an atomic boolean as you just check for future completeness
skisel-da
reviewed
Feb 13, 2023
Comment on lines
+743
to
+750
| timer.schedule( | ||
| new TimerTask { | ||
| override def run(): Unit = { | ||
| Await.result(finishLongTakingTask.future, Duration(10, "seconds")) | ||
| } | ||
| }, | ||
| 0L, | ||
| ) |
There was a problem hiding this comment.
nitpick: the whole timer.schedule could be extracted into a small def and reused 3 more times :) not a big concern though
skisel-da
approved these changes
Feb 13, 2023
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.
After this PR the TimerResourceOwner will make sure that none of the scheduled task are running, after the Resource finished releasing. Also adds asynchronous test as evidence.
As the system is going down, and we reach the DbDispatcher, then we first tear down the executor pool, then the timer, then the hikari connection pool.
before: after the timer had gone down, we still could have had a running db check, which was racing with the Hikari tear down, It could have resulted in warnings in logs pointing to some error with connection.
after: no such warnings: Hikari only goes sown after the timer released, and all timer task finished
[CHANGELOG_BEGIN]
[CHANGELOG_END]