Fix 288: close handle when remWatch open in getIno #418
Conversation
|
@shogo82148 Could you please approve this? #288 have to be fixed since this is handle leak. On Windows, the directory can not be possible to delete manually if any handles are opend for the directory. |
|
Is there a reason to only run this test on Windows? |
|
This is a bug occured only on Windows. |
nshalman
left a comment
nshalman
left a comment
There was a problem hiding this comment.
Unless I'm really misunderstanding something, I think this can be made into an integration test which is generally preferable. Comment inline explaining how I think it could be done.
windows_test.go
Outdated
| if err := watcher.Remove(testDir); err != nil { | ||
| t.Fatalf("Could not remove the watch: %v\n", err) | ||
| } | ||
| if err := watcher.remWatch(testDir); err == nil { |
There was a problem hiding this comment.
If this line was also watcher.Remove I think this should be safe to put back in as an integration test across all OSes. It works correctly on my as-yet-not-merged illumos tree, and I think we should do our best that most tests should be run across all platforms rather than specific to a single one...
There was a problem hiding this comment.
Uhhh. Those test failures are confusing me. I wonder if the official API is to allow repeated removal and just ignore it if it's not watched? Let me do some digging. Thank you for being responsive! Sorry for the delay, but I think we can get this right.
There was a problem hiding this comment.
Ah, I see you you fixed it!
There was a problem hiding this comment.
There are differences for the error messages on some OSs. Should we correct to same?
There was a problem hiding this comment.
I think it's helpful that they are specific about the underlying implementation. If anything I think using the type system to return a typed error would make sense, but I don't think that should be in-scope for this PR.
We should only need the integration test and the fix (having the duplicate test for Windows could lead to confusion later, in my opinion.)
nshalman
left a comment
nshalman
left a comment
There was a problem hiding this comment.
I think we're almost there. Unless there's a really good reason to have a duplicate test on Windows I think we just need the fixed up integration test and the fix for Windows.
Can you confirm that the integration test fails on Windows without the fix?
windows_test.go
Outdated
| if err := watcher.Remove(testDir); err != nil { | ||
| t.Fatalf("Could not remove the watch: %v\n", err) | ||
| } | ||
| if err := watcher.remWatch(testDir); err == nil { |
There was a problem hiding this comment.
I think it's helpful that they are specific about the underlying implementation. If anything I think using the type system to return a typed error would make sense, but I don't think that should be in-scope for this PR.
We should only need the integration test and the fix (having the duplicate test for Windows could lead to confusion later, in my opinion.)
|
I don't think this test catches the corresponding error. I tried pushing a branch with the test but not the fix and the tests still pass: https://github.com/nshalman/fsnotify/actions/runs/1709124231 I think we should look more closely at https://gist.github.com/timshannon/603f92824c5294269797 and #42 My naive attempt to create a test matching that gist also failed (the test doesn't fail): nshalman@7867de0 |
|
Thank you. I merged your patch. |
|
Sorry, I don't think I was clear. My test is not diagnostically useful in its current form. It passes even without the proposed fix. :( For a regression test to be useful, it should fail if run before the fix is applied. I could use some help writing an effective regression test. |
|
I'll look in later. |
|
I just merged this as it's been well over a year; if more work needs to be done then this can be a new PR. |
NOTE: Please do not open a pull request that adds or changes the public API without giving consideration to all supported operating systems.
What does this pull request do?
Closing #288
Where should the reviewer start?
Commits are contained in this PR too.
How should this be manually tested?
No