8000 pkg/filenotify/poller fixes by kolyshkin · Pull Request #37734 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 3 commits into from
Sep 1, 2018
Merged

pkg/filenotify/poller fixes #37734

merged 3 commits into from
Sep 1, 2018

Conversation

kolyshkin
Copy link
Contributor
@kolyshkin kolyshkin commented Aug 29, 2018

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.

three-bugs-with-green-umbrellas-hd-insects-fun-wallpaper

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>
@kolyshkin
Copy link
Contributor Author

CI failures look unrelated, both are known flaky test suspects

experimental:

01:38:16.790 FAIL: docker_cli_swarm_test.go:1376: DockerSwarmSuite.TestSwarmClusterRotateUnlockKey

power:

01:51:36.171 FAIL: docker_api_swarm_test.go:359: DockerSwarmSuite.TestAPISwarmRaftQuorum

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
Copy link
codecov bot commented Aug 30, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@64b7575). Click here to learn what that means.
The diff coverage is 50%.

@@            Coverage Diff            @@
##             master   #37734   +/-   ##
=========================================
  Coverage          ?   36.06%           
=========================================
  Files             ?      609           
  Lines             ?    45063           
  Branches          ?        0           
=========================================
  Hits              ?    16254           
  Misses            ?    26582           
  Partials          ?     2227

Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor
@tiborvass tiborvass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Member

This bug was revealed because since #37412, this code is used for Windows logging

Copy link
Contributor
@AntaresS AntaresS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin
Copy link
Contributor Author
kolyshkin commented Aug 30, 2018

This bug

Actually three (as depicted 😺)

select {
case <-time.After(watchWaitTime):
Copy link
Contributor

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.

Copy link
Contributor

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 ?

Copy link
Contributor Author
@kolyshkin kolyshkin Sep 1, 2018

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.

Copy link
Member

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.

Copy link
Member
@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0