-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stop and join watchdog observer #11706
Conversation
This is in accordance with the example. For inotify at least the `.stop()` call ends up calling the removed unschedule call anyways. https://github.com/gorakhargosh/watchdog/tree/72f2eb7203659c11c4f0703c818dcb88fe527e99#example-api-usage
Tracking flakes while rerunning this. https://github.com/Chia-Network/chia-blockchain/runs/6646220657?check_suite_focus=true
|
PyPI went down for a bit. |
https://github.com/Chia-Network/chia-blockchain/runs/6646942280?check_suite_focus=true
|
https://github.com/Chia-Network/chia-blockchain/runs/6647224077?check_suite_focus=true
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aok
Draft for:
Note possible upstream fix for the issue at gorakhargosh/watchdog#895 being tested in #11710.
This is in accordance with the watchdog example. For inotify at least the
.stop()
call ends up calling the remove unschedule call anyways.https://github.com/gorakhargosh/watchdog/tree/72f2eb7203659c11c4f0703c818dcb88fe527e99#example-api-usage
This appears to be only used in test code. This lowers the bar for trying this out in
main
to see how it holds up but also raises the question of why we aren't doing any shutdown at all for normal runs, seemingly.This was found while looking into the following traceback we occasionally get in CI.
I don't have any historical data on how often this was happening so the several CI reruns below aren't any sort of proof that the issue is actually fixed. I just figured I'd give it a chance to still pop up and also collect a list of other flakes that are still hanging around. So far it looks like CI is in a pretty good state right now in terms of flakes. But yeah, the bad file descriptor issue did still show up with just this change.