Skip to content

Create inotify fd with close-on-exec#178

Merged
nathany merged 2 commits intofsnotify:masterfrom
pattyshack:close-on-exec
Oct 11, 2016
Merged

Create inotify fd with close-on-exec#178
nathany merged 2 commits intofsnotify:masterfrom
pattyshack:close-on-exec

Conversation

@pattyshack
Copy link
Copy Markdown

@pattyshack pattyshack commented Oct 5, 2016 •
edited by nathany
Loading

resolves #155

(originally from #174)

nathany
nathany suggested changes Oct 5, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@nathany nathany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :-)

inotify.go Outdated
func NewWatcher() (*Watcher, error) {
// Create inotify fd
fd, errno := unix.InotifyInit()
fd, errno := unix.InotifyInit1(syscall.IN_CLOEXEC)
Copy link
Copy Markdown
Contributor

@nathany nathany Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be unix.IN_CLOEXEC since unix is a more up-to-date version of the syscall package.

@nathany nathany added the linux label Oct 5, 2016
@nathany
Copy link
Copy Markdown
Contributor

nathany commented Oct 5, 2016

Waiting on feedback/testing for ARM and PPC64 before merging.

@nathany
Copy link
Copy Markdown
Contributor

nathany commented Oct 5, 2016

Can you provide steps to manually test that file descriptors are no longer leaking with this change?

@nathany nathany mentioned this pull request Oct 5, 2016
Closed
@pattyshack
Copy link
Copy Markdown
Author

pattyshack commented Oct 5, 2016 

$ cat test.go
package main

import (
    "fmt"
    "log"
    "os"
    "os/exec"
    "time"

    "github.com/fsnotify/fsnotify"
)

func main() {
    if len(os.Args) == 1 { // parent
        watcher, err := fsnotify.NewWatcher()
        if err != nil {
            log.Fatal(err)
        }
        defer watcher.Close()

        child := exec.Command(os.Args[0], "child")
        err = child.Start()
        if err != nil {
            log.Fatal(err)
        }

        fmt.Println("Parent", os.Getpid(), "child", child.Process.Pid)
    }

    fmt.Println(os.Args, "going into sleep")
    for {
        time.Sleep(time.Second)
    }
}

$ ./test 
Parent 25174 child 25179
[./test] going into sleep

$ lsof -p 25174
COMMAND   PID    USER   FD      TYPE DEVICE SIZE/OFF     NODE NAME
test    25174 patrick  cwd       DIR  252,1     4096 36051491 /home/patrick/git/go/src/github.com/pattyshack/fsnotify/test
test    25174 patrick  rtd       DIR  252,1     4096        2 /
test    25174 patrick  txt       REG  252,1  2203205 12591190 /home/patrick/git/go/src/github.com/pattyshack/fsnotify/test/test
test    25174 patrick    0u      CHR  136,6      0t0        9 /dev/pts/6
test    25174 patrick    1u      CHR  136,6      0t0        9 /dev/pts/6
test    25174 patrick    2u      CHR  136,6      0t0        9 /dev/pts/6
test    25174 patrick    3r  a_inode   0,11        0     8012 inotify
test    25174 patrick    4u  a_inode   0,11        0     8012 [eventpoll]
test    25174 patrick    5r     FIFO   0,10      0t0   546148 pipe
test    25174 patrick    6w     FIFO   0,10      0t0   546148 pipe
$ lsof -p 25179
COMMAND   PID    USER   FD      TYPE DEVICE SIZE/OFF     NODE NAME
test    25179 patrick  cwd       DIR  252,1     4096 36051491 /home/patrick/git/go/src/github.com/pattyshack/fsnotify/test
test    25179 patrick  rtd       DIR  252,1     4096        2 /
test    25179 patrick  txt       REG  252,1  2203205 12591190 /home/patrick/git/go/src/github.com/pattyshack/fsnotify/test/test
test    25179 patrick    0r      CHR    1,3      0t0        6 /dev/null
test    25179 patrick    1w      CHR    1,3      0t0        6 /dev/null
test    25179 patrick    2w      CHR    1,3      0t0        6 /dev/null
test    25179 patrick    4u  a_inode   0,11        0     8012 [eventpoll]
test    25179 patrick    5r     FIFO   0,10      0t0   546148 pipe
test    25179 patrick    6w     FIFO   0,10      0t0   546148 pipe

Notice the child process does not have an inotify fd

@clnperez
Copy link
Copy Markdown

clnperez commented Oct 7, 2016 •
edited
Loading

I also tested this on ppc64le and it looks like both parent and child have an inotify fd:

 ~> lsof -p 2338
COMMAND    PID    USER   FD      TYPE DEVICE SIZE/OFF    NODE NAME
fsnotify_ 2338 christy  cwd       DIR  253,2     4096  261642 /home/christy
fsnotify_ 2338 christy  rtd       DIR  253,2     4096       2 /
fsnotify_ 2338 christy  txt       REG  253,2  2351144 2373750 /tmp/go-build372706786/command-line-arguments/_obj/exe/fsnotify_inotify
fsnotify_ 2338 christy    0u      CHR  136,0      0t0       3 /dev/pts/0
fsnotify_ 2338 christy    1u      CHR  136,0      0t0       3 /dev/pts/0
fsnotify_ 2338 christy    2u      CHR  136,0      0t0       3 /dev/pts/0
fsnotify_ 2338 christy    3r  a_inode   0,11        0    6637 inotify
fsnotify_ 2338 christy    4u  a_inode   0,11        0    6637 [eventpoll]
fsnotify_ 2338 christy    5r     FIFO   0,10      0t0  432376 pipe
fsnotify_ 2338 christy    6w     FIFO   0,10      0t0  432376 pipe
 ~> lsof -p 2343
COMMAND    PID    USER   FD      TYPE DEVICE SIZE/OFF    NODE NAME
fsnotify_ 2343 christy  cwd       DIR  253,2     4096  261642 /home/christy
fsnotify_ 2343 christy  rtd       DIR  253,2     4096       2 /
fsnotify_ 2343 christy  txt       REG  253,2  2351144 2373750 /tmp/go-build372706786/command-line-arguments/_obj/exe/fsnotify_inotify
fsnotify_ 2343 christy    0r      CHR    1,3      0t0       6 /dev/null
fsnotify_ 2343 christy    1w      CHR    1,3      0t0       6 /dev/null
fsnotify_ 2343 christy    2w      CHR    1,3      0t0       6 /dev/null
fsnotify_ 2343 christy    3r  a_inode   0,11        0    6637 inotify
fsnotify_ 2343 christy    4u  a_inode   0,11        0    6637 [eventpoll]
fsnotify_ 2343 christy    5r     FIFO   0,10      0t0  432376 pipe
fsnotify_ 2343 christy    6w     FIFO   0,10      0t0  432376 pipe

@nathany
Copy link
Copy Markdown
Contributor

nathany commented Oct 7, 2016

@clnperez That's odd. I wonder if the IN_CLOEXEC flag isn't getting passed in properly or what could be causing the discrepancy on ppc.

@nathany
Copy link
Copy Markdown
Contributor

nathany commented Oct 7, 2016 •
edited
Loading

I don't know why this wouldn't work. https://github.com/golang/sys/blob/master/unix/zsyscall_linux_ppc64le.go#L629

IN_CLOEXEC is defined with the same value as other platforms.
https://github.com/golang/sys/blob/master/unix/zerrors_linux_ppc64le.go#L510

So I wonder where the problem is.

@clnperez
Copy link
Copy Markdown

clnperez commented Oct 7, 2016 •
edited
Loading

Yah, I don't know either. I had to add the syscalls for Pipe2 and InotifyInit, but InotifyInit1 is already there, so, hmm. I hope it just isn't something wonky on my system.

@clnperez
Copy link
Copy Markdown

clnperez commented Oct 7, 2016

Oh man. I forgot to rebuild fsnotify with this patch after I got my sys updated (since it was an older copy without the other syscalls added for ppc64le). Kiiiind of important. Sorry about the noise. It works fine on ppc64le. :)

@pattyshack
Copy link
Copy Markdown
Author

pattyshack commented Oct 10, 2016

ping (sounds like ppc64 is ok. is there any other blocker?)

@nathany nathany merged commit 11054bc into fsnotify:master Oct 11, 2016
@pattyshack
Copy link
Copy Markdown
Author

pattyshack commented Oct 11, 2016

thx

@pwaller pwaller mentioned this pull request Sep 1, 2017
Merged
@jefferyto jefferyto mentioned this pull request Jul 23, 2019
Merged
@dims dims mentioned this pull request Aug 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nathany nathany nathany approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

linux

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

watchers leak inotify file descriptor when process fork/execs a child

3 participants

@pattyshack @nathany @clnperez