-
Notifications
You must be signed in to change notification settings - Fork 18.8k
pkg/filenotify/poller fixes #37734
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
pkg/filenotify/poller fixes #37734
Conversation
In case of errors, the file descriptor is never closed. Fix it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is no need to wait for up to 200ms in order to close the file descriptor once the chClose is received. This commit might reduce the chances for occasional "The process cannot access the file because it is being used by another process" error on Windows, where an opened file can't be removed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
CI failures look unrelated, both are known flaky test suspects experimental:
power:
|
The code in Close() that removes the watches was not working, because it first sets `w.closed = true` and then calls w.close(), which starts with ``` if w.closed { return errPollerClosed } ``` Fix by setting w.closed only after calling w.remove() for all the files being watched. While at it, remove the duplicated `delete(w.watches, name)` code. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #37734 +/- ##
=========================================
Coverage ? 36.06%
=========================================
Files ? 609
Lines ? 45063
Branches ? 0
=========================================
Hits ? 16254
Misses ? 26582
Partials ? 2227 |
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.
LGTM
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.
LGTM
This bug was revealed because since #37412, this code is used for Windows logging |
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.
LGTM
Actually three (as depicted 😺) |
select { | ||
case <-time.After(watchWaitTime): |
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.
You might have stray timers because of After
, it's better to use NewTimer
+ Stop
later I think.
There was a problem hiding this comment.
Choose a 8000 reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean Stop in defer ?
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.
Will the timer still finish? If yes, in here the watchWaitTime is 200ms and the watcher.Close() is not something called too often so if the timer will still finish I guess it's not big of an issue.
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.
Agree this should be fine.
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.
LGTM
A couple of fixes to issues I found while reading the code. Might help reduce the number of mysterious bugs on Windows, where poller is used for logs. See individual commits for descriptions.