Fix EventFileWriter deadlock on exception in background thread #6168
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
|
@groszewn the issue with build seems to be unrelated and I fixed the linter warning. Could you please re-run the workflow? |
|
@groszewn sorry, now fixed the linter warning for real: Could you please restart CI once again? |
groszewn
requested changes
Jan 31, 2023
There was a problem hiding this comment.
Hi @crassirostris, really appreciate the well-documented issue and PR! Left a few comments.
Signed-off-by: Mik Vyatskov <[email protected]>
crassirostris
commented
Feb 1, 2023
There was a problem hiding this comment.
Thanks a lot for the thorough review, @groszewn! Fixed behavior for flush, could you please take another look?
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this pull request
Mar 27, 2023
…rflow#6168) ## Motivation for features / changes To address tensorflow#6167 ## Technical description of changes This is a bug fix for possible deadlock when writing events through `EventFileWriter`. The PR adds logic in `_AsyncWriterThread` to catch exception to propagate it to the calling thread and adds logic to `_AsyncWriter` to propagate exception raised in `_AsyncWriterThread` ## Detailed steps to verify changes work correctly (as executed by you) New unit test that is not passing on master ## Alternate designs / implementations considered * Instead of popping an item from the queue on exception, it's possible to make `wait`/`flush` methods re-check the status periodically * Instead of raising an exception in the foreground thread, it's possible to ignore the raised exception altogether and just start dropping events * It's possible to drop the data after it cannot be added to the queue for a certain period of time Signed-off-by: Mik Vyatskov <[email protected]>
dna2github
pushed a commit
to dna2fork/tensorboard
that referenced
this pull request
May 1, 2023
…rflow#6168) ## Motivation for features / changes To address tensorflow#6167 ## Technical description of changes This is a bug fix for possible deadlock when writing events through `EventFileWriter`. The PR adds logic in `_AsyncWriterThread` to catch exception to propagate it to the calling thread and adds logic to `_AsyncWriter` to propagate exception raised in `_AsyncWriterThread` ## Detailed steps to verify changes work correctly (as executed by you) New unit test that is not passing on master ## Alternate designs / implementations considered * Instead of popping an item from the queue on exception, it's possible to make `wait`/`flush` methods re-check the status periodically * Instead of raising an exception in the foreground thread, it's possible to ignore the raised exception altogether and just start dropping events * It's possible to drop the data after it cannot be added to the queue for a certain period of time Signed-off-by: Mik Vyatskov <[email protected]>
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.
Motivation for features / changes
To address #6167
Technical description of changes
This is a bug fix for possible deadlock when writing events through
EventFileWriter. The PR adds logic in_AsyncWriterThreadto catch exception to propagate it to the calling thread and adds logic to_AsyncWriterto propagate exception raised in_AsyncWriterThreadDetailed steps to verify changes work correctly (as executed by you)
New unit test that is not passing on master
Alternate designs / implementations considered
wait/flushmethods re-check the status periodically