Do not suppress Chmod on non-existent file #35
Conversation
Currently fsnotify suppresses a Chmod event if the file does not exist when the event is received. This makes it impossible to use fsnotify to detect when an opened file is removed. In such case the Linux kernel sends IN_ATTRIB event, as described in inotify(7) man page: > IN_ATTRIB (*) > Metadata changed—for example, permissions (e.g., chmod(2)), > timestamps (e.g., utimensat(2)), extended attributes (setx‐ > attr(2)), link count (since Linux 2.6.25; e.g., for the tar‐ > get of link(2) and for unlink(2)), and user/group ID (e.g., > chown(2)). (to clarify, in this very case it's link count that changes). To fix: * Modify the code to only suppress MODIFY and CREATE events. * Add a test case to verify Chmod event is delivered. While at it, fix the comment in ignoreLinux() to use the up-to-date terminology (event ops). A test case is added. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
WalkthroughThis update refines the event handling in a Go project by shifting the focus of Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // If the event is Create or Write, the file must exist, or the | ||
| // event will be suppressed. | ||
| // *Note*: this was put in place because it was seen that a Write | ||
| // event was sent after the Remove. This ignores the Write and | ||
| // assumes a Remove will come or has come if the file doesn't exist. | ||
| if e.Op&Create != 0 || e.Op&Write != 0 { |
There was a problem hiding this comment.
The update to the ignoreLinux function to focus on suppressing Create or Write events for non-existent files aligns well with the PR's objectives. This change ensures that Chmod events, which can indicate a file's removal, are not suppressed, enhancing the library's ability to detect file deletions.
One minor suggestion for improvement is to clarify the comment further by explicitly stating that Chmod events are not suppressed by this logic, making it clear for future maintainers why this specific change was made.
// If the event is Create or Write, the file must exist, or the
// event will be suppressed. This logic specifically allows Chmod
// events, which can indicate a file's removal, to be processed.
// *Note*: This was put in place because it was observed that a Write
// event could be sent after the Remove event. This logic ignores the Write
// and assumes a Remove will come or has come if the file doesn't exist.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // If the event is Create or Write, the file must exist, or the | |
| // event will be suppressed. | |
| // *Note*: this was put in place because it was seen that a Write | |
| // event was sent after the Remove. This ignores the Write and | |
| // assumes a Remove will come or has come if the file doesn't exist. | |
| if e.Op&Create != 0 || e.Op&Write != 0 { | |
| // If the event is Create or Write, the file must exist, or the | |
| // event will be suppressed. This logic specifically allows Chmod | |
| // events, which can indicate a file's removal, to be processed. | |
| // *Note*: This was put in place because it was observed that a Write | |
| // event could be sent after the Remove event. This logic ignores the Write | |
| // and assumes a Remove will come or has come if the file doesn't exist. | |
| if e.Op&Create != 0 || e.Op&Write != 0 { |
|
|
||
| func TestInotifyDeleteOpenedFile(t *testing.T) { | ||
| testDir := tempMkdir(t) | ||
| defer os.RemoveAll(testDir) | ||
|
|
||
| testFile := filepath.Join(testDir, "testfile") | ||
|
|
||
| // create and open a file | ||
| fd, err := os.Create(testFile) | ||
| if err != nil { | ||
| t.Fatalf("Create failed: %v", err) | ||
| } | ||
| defer fd.Close() | ||
|
|
||
| w, err := NewWatcher() | ||
| if err != nil { | ||
| t.Fatalf("Failed to create watcher: %v", err) | ||
| } | ||
| defer w.Close() | ||
|
|
||
| err = w.Add(testFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to add watch for %s: %v", testFile, err) | ||
| } | ||
|
|
||
| checkEvent := func(exp Op) { | ||
| select { | ||
| case event := <-w.Events: | ||
| t.Logf("Event received: %s", event.Op) | ||
| if event.Op != exp { | ||
| t.Fatalf("Event expected: %s, got: %s", exp, event.Op) | ||
| } | ||
| case <-time.After(100 * time.Millisecond): | ||
| t.Fatalf("Expected %s event not received", exp) | ||
| } | ||
| } | ||
|
|
||
| // Remove the (opened) file, check Chmod event (notifying | ||
| // about file link count change) is received | ||
| err = os.Remove(testFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to remove file: %s", err) | ||
| } | ||
| checkEvent(Chmod) | ||
|
|
||
| // Close the file, check Remove event is received | ||
| fd.Close() | ||
| checkEvent(Remove) | ||
| } |
There was a problem hiding this comment.
The TestInotifyDeleteOpenedFile function is well-implemented and effectively tests the delivery of Chmod and Remove events upon file deletion. This aligns with the PR's objectives and enhances the test coverage for the fsnotify library.
One suggestion for improvement is to ensure that the watcher (w) is closed even if the test fails before reaching the defer w.Close() statement. This can be achieved by moving the watcher creation and defer statement to the beginning of the test function.
+ w, err := NewWatcher()
+ if err != nil {
+ t.Fatalf("Failed to create watcher: %v", err)
+ }
+ defer w.Close()
+
fd, err := os.Create(testFile)
if err != nil {
t.Fatalf("Create failed: %v", err)
}
defer fd.Close()
- w, err := NewWatcher()
- if err != nil {
- t.Fatalf("Failed to create watcher: %v", err)
- }
- defer w.Close()This change ensures that the watcher is always properly closed, preventing potential resource leaks during test execution.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func TestInotifyDeleteOpenedFile(t *testing.T) { | |
| testDir := tempMkdir(t) | |
| defer os.RemoveAll(testDir) | |
| testFile := filepath.Join(testDir, "testfile") | |
| // create and open a file | |
| fd, err := os.Create(testFile) | |
| if err != nil { | |
| t.Fatalf("Create failed: %v", err) | |
| } | |
| defer fd.Close() | |
| w, err := NewWatcher() | |
| if err != nil { | |
| t.Fatalf("Failed to create watcher: %v", err) | |
| } | |
| defer w.Close() | |
| err = w.Add(testFile) | |
| if err != nil { | |
| t.Fatalf("Failed to add watch for %s: %v", testFile, err) | |
| } | |
| checkEvent := func(exp Op) { | |
| select { | |
| case event := <-w.Events: | |
| t.Logf("Event received: %s", event.Op) | |
| if event.Op != exp { | |
| t.Fatalf("Event expected: %s, got: %s", exp, event.Op) | |
| } | |
| case <-time.After(100 * time.Millisecond): | |
| t.Fatalf("Expected %s event not received", exp) | |
| } | |
| } | |
| // Remove the (opened) file, check Chmod event (notifying | |
| // about file link count change) is received | |
| err = os.Remove(testFile) | |
| if err != nil { | |
| t.Fatalf("Failed to remove file: %s", err) | |
| } | |
| checkEvent(Chmod) | |
| // Close the file, check Remove event is received | |
| fd.Close() | |
| checkEvent(Remove) | |
| } | |
| func TestInotifyDeleteOpenedFile(t *testing.T) { | |
| testDir := tempMkdir(t) | |
| defer os.RemoveAll(testDir) | |
| testFile := filepath.Join(testDir, "testfile") | |
| w, err := NewWatcher() | |
| if err != nil { | |
| t.Fatalf("Failed to create watcher: %v", err) | |
| } | |
| defer w.Close() | |
| // create and open a file | |
| fd, err := os.Create(testFile) | |
| if err != nil { | |
| t.Fatalf("Create failed: %v", err) | |
| } | |
| defer fd.Close() | |
| err = w.Add(testFile) | |
| if err != nil { | |
| t.Fatalf("Failed to add watch for %s: %v", testFile, err) | |
| } | |
| checkEvent := func(exp Op) { | |
| select { | |
| case event := <-w.Events: | |
| t.Logf("Event received: %s", event.Op) | |
| if event.Op != exp { | |
| t.Fatalf("Event expected: %s, got: %s", exp, event.Op) | |
| } | |
| case <-time.After(100 * time.Millisecond): | |
| t.Fatalf("Expected %s event not received", exp) | |
| } | |
| } | |
| // Remove the (opened) file, check Chmod event (notifying | |
| // about file link count change) is received | |
| err = os.Remove(testFile) | |
| if err != nil { | |
| t.Fatalf("Failed to remove file: %s", err) | |
| } | |
| checkEvent(Chmod) | |
| // Close the file, check Remove event is received | |
| fd.Close() | |
| checkEvent(Remove) | |
| } |
port of fsnotify/fsnotify#260
Currently fsnotify suppresses a Chmod event if the file does not exist when the event is received.
This makes it impossible to use fsnotify to detect when an opened file is removed. In such case the Linux kernel sends IN_ATTRIB event, as described in inotify(7) man page:
(to clarify, in this very case it's link count that changes).
To fix:
While at it, fix the comment in ignoreLinux() to use the up-to-date terminology (event ops).
A test case is added.
Summary by CodeRabbit
CreateorWriteevents for better accuracy in monitoring file changes.