Skip to content

pkg/filenotify/poller fixes#37734

Merged
cpuguy83 merged 3 commits intomoby:masterfrom
kolyshkin:poller
Sep 1, 2018
Merged

pkg/filenotify/poller fixes#37734
cpuguy83 merged 3 commits intomoby:masterfrom
kolyshkin:poller

Conversation

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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

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

kolyshkin commented Aug 30, 2018

This bug

Actually three (as depicted 😺)

for {
time.Sleep(watchWaitTime)
select {
case <-time.After(watchWaitTime):
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Choose a 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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