avoid deadlocks on close. Fixes #187 #188
Conversation
|
probably addresses #145 as well. |
|
confirmed. this patch makes test in #145 pass. |
| if index > 10 { | ||
| return | ||
| } | ||
| index += 1 |
There was a problem hiding this comment.
should replace index += 1 with index++
| select { | ||
| case event := <-watcher.Events: | ||
| log.Println("event:", event) | ||
| notifications += 1 |
There was a problem hiding this comment.
should replace notifications += 1 with notifications++
|
Ping. Please merge this. |
raw channel work (not inside a select) should always be prohibited in production code, as it readily causes deadlocks on shutdown. Also adds the test TestWatcherClose from #145. This request duplicates that test, with two lines fixed to address the houndcli-bot review concerns. Fixes #187 Fixes #145
There was a problem hiding this comment.
@glycerine I'd like to get this merged ASAP as its a serious bug. If it's cool with you may I take over?
@nathany I believe this covers #211 and #145 so after this is merged, those should probably be closed.
| if err != nil { | ||
| select { | ||
| case w.Errors <- err: | ||
| case <-w.done: |
There was a problem hiding this comment.
given the only way this function can return is if w.done is closed, I think this should instead be the default case.
|
|
||
| func consumeEvent(t *testing.T, watcher *Watcher, wg *sync.WaitGroup) { | ||
| defer func() { | ||
| //time.Sleep(time.Second) |
There was a problem hiding this comment.
debugging line should be removed.
| func TestWatcherClose(t *testing.T) { | ||
| dir, err := ioutil.TempDir("./", "example") | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
this panic and the others should be t.Fatal
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ioutil.TempFile(dir, "example") |
| if err != nil { | ||
| panic(err) | ||
| } | ||
| _, err = ioutil.TempFile(dir, "example") |
| watcher.Close() | ||
| wg.Done() | ||
| }() | ||
| log.Println("Came here") |
| maxNotifications := 1 | ||
| for { | ||
| if notifications >= maxNotifications { | ||
| log.Println("Reached max notifications") |
| } | ||
| select { | ||
| case event := <-watcher.Events: | ||
| log.Println("event:", event) |
| index++ | ||
| } | ||
| } | ||
| func TestWatcherClose(t *testing.T) { |
There was a problem hiding this comment.
this test could be made much simpler. We just need the watcher to be listening on some file, modify the file and then close the watcher without listening on either the errors or events channels.
| // Send "quit" message to the reader goroutine: | ||
| w.done <- true | ||
| // send a "quit" message to the reader goroutine | ||
| close(w.done) |
There was a problem hiding this comment.
we should change w.done to be a chan struct{}.
by all means. |
Raw channel work (not inside a select) is subject to deadlock on shutdown.
Hence Close() can deadlock.
This is already fixed in the linux inotify.go code.
Fixes #187