create inotify fd with close-on-exec + deflake inotify stress test #174
Conversation
|
Thanks for submitting this. Been on my list of things to push back for over a year. |
|
Looks like the project's travis config is broken (the linter doesn't build in go 1.5). Otherwise, the change seems fine |
|
I merged #175. Want to rebase and give this another go on Travis? |
nathany
left a comment
nathany
left a comment
There was a problem hiding this comment.
Thanks Patrick.
I'm not 100% sure on the InotifyInit1 change yet -- we may need some people with ARM and IBM Power devices to look at it first.
Is it possible to deflake the test independently and come back to the close-on-exec change, or is that necessary for deflakery?
| func NewWatcher() (*Watcher, error) { | ||
| // Create inotify fd | ||
| fd, errno := unix.InotifyInit() | ||
| fd, errno := unix.InotifyInit1(syscall.IN_CLOEXEC) |
There was a problem hiding this comment.
We need to be careful with this change, possibly testing it manually as Travis CI doesn't give us coverage of arm and ppc64 architectures that fsnotify currently supports.
There was a problem hiding this comment.
Hoping the Go team will help us out with that, but I'm not sure when they'll have time. golang/go#17312
| } | ||
| }() | ||
| doneChan := make(chan struct{}) | ||
| errChan := make(chan error, 1000) |
There was a problem hiding this comment.
does this channel need to be buffered?
| errChan <- fmt.Errorf("Remove failed: %v", err) | ||
| } | ||
|
|
||
| time.Sleep(time.Millisecond) |
There was a problem hiding this comment.
I'm curious why the 1ms sleep is needed.
There was a problem hiding this comment.
This stress test is pretty lengthy, so it might be nice to add a few more comments for future maintainers.
|
(i totally messed up my branch. will send another pull request) |
|
Both locally and your remote? 😢 A new PR is fine by me though. |
3 participants
(on behalf of @msolo) inotify fd should not leak to children processes (this matches the kqueue implementation behavior).