Inotify #39

chamaken · 2014-09-11T10:15:46Z

I think I could fix an inotify issue - https://code.google.com/p/go/issues/detail?id=2483
see https://github.com/chamaken/inotify
And then I am glad if I can backport it to your fsnotify. Would you review the patchs?

If I am watching a directory, and delete it, and recreate it,
and watch it again, any events after that are hosed:

the event.Name is incomplete, with the path stripped out e.g.
instead of /home/myname/a/b/c/d.txt, I get /d.txt
instead of /home/myname/a/b/c/, I get ""

RemoveWatch fails with message: invalid argument

Close() doesn't really close. Most times, the reader goroutine hangs.

to checks internal maps len

fail in checking maps len

inotify(7): IN_IGNORED Watch was removed explicitly (inotify_rm_watch(2)) or automatically (file was deleted, or file system was unmounted). watches is removed in ignoreLinux() and paths too. introduce condv to sync Remove() with ignoreLinux().

to finish go routine.

nathany · 2014-09-11T16:43:56Z

inotify.go

 	}
+	event = &syscall.EpollEvent{syscall.EPOLLIN, int32(rp.Fd()), 0}


I'm not familiar with this stuff. Some comments would help.

So you're setting up epoll on the file descriptor for inotify events... and then this os.Pipe() file descriptor as well

So you know though readEvents() blocks so that we need to break it to close gracefully.
epoll can be used which descriptor can be handled from which it was registerd like select(2)
but FdSet which is params of syscall.Select() has no handling function - FD_SET macro.

I introduced epoll and os.Pipe() just notifying to epoll that watcher.Close() method is called.
The flow is - watcher.Close() send bool through watcher.done channel, inline go routine receives it and closes the pipe, then epoll notices the pipe is closed and breaks loop, returns.

Introducing epoll seems overdoing but I could not find more better way, sorry.

It might be just the right thing, I'm just still learning Linux stuff since taking over maintenance of the project. Thanks for explaining the choice of epoll rather than select.

I do want to make sure that the code is understandable and maintainable going forward.

Before I merge this in, take a look at #5 and the links to when @howeyc added and remove select(). Let me know what you think.

it seems that FD_ macros were wrong. using original, 660b1c4 did not work but refering http://play.golang.org/p/rIuaDUzLXw, 0ab96cb worked. I will rewrite using select, timeout if you prefer.

By the way patches I've posted has strange EOF handling and needs a bit clean up, could you let me rewrite it? you can see these chamaken@d6718be#diff-9dee340ab29d1c1db9e272db38706348 and chamaken@3687df3#diff-9dee340ab29d1c1db9e272db38706348

nathany · 2014-09-11T17:01:29Z

Thanks for working on this @chamaken. I left a few comments.

handle as same as another error.

nathany · 2014-09-13T13:48:21Z

Feel free to add more commits here or open a new pull request.

Epoll vs. select is up to you. Whichever works better.

Thanks @chamaken

chamaken · 2014-09-14T02:06:50Z

I close and reopen by mistake, sorry.
I prefer using pipe fd notification than timeout. Talking about epoll vs. select
I think there are three issues.

select requires cgo, that's say built binary will be dynamically linked
epoll was developed for large number of file descriptors
(It seems) you are familiar with select more than epoll

would you, maintainer decide which one is suitable?

nathany · 2014-09-14T15:04:07Z

@chamaken I'm not familiar with either, so I don't know.

We could try epoll and see how it goes. We could also request a code review from people on golang-nuts and see what we get back?

chamaken · 2014-09-14T15:26:17Z

I agree. Thank you for your suggestion. @nathany

dmitshur · 2014-09-15T06:21:30Z

inotify.go

 // Watcher watches a set of files, delivering events to a channel.
 type Watcher struct {
 	Events   chan Event
 	Errors   chan error
 	mu       sync.Mutex        // Map access
+        cv       *sync.Cond        // sync removing on rm_watch with IN_IGNORE


It looks like it needs to be gofmted.

I don't mind doing minor cleanup as part of my merging process. More concerned with functionality, structure and maintainability.

Of course, running gofmt and checking for typos is always appreciated. :-)

nathany · 2014-09-15T06:44:12Z

@chamaken Since I don't know when I'll read my Linux book and figure this stuff out, I put out a request for code review on golang-nuts.

Thanks to shurcooL, I was able to grasp I should gofmt.

nathany · 2014-09-24T13:59:03Z

Sorry for letting this sit for so long.

Even though this isn't changing the API, I'm beginning to think that such big internal changes should also warrant a new major version and a release candidate, which is something gopkg.in is considering support for: niemeyer/gopkg#25.

I'm still figuring out exactly what this should look like.

chamaken · 2014-09-25T00:50:04Z

Thank you for telling me. @nathany
I am OK with that.

inotify(7) says: When all file descriptors referring to an inotify instance have been closed, the underlying object and its resources are freed for reuse by the kernel; all associated watches are automatically freed.

I had forgotten.

nathany · 2014-11-15T06:22:43Z

Hi @chamaken,

I think this is too big of a change for right now. Personally, I still haven't had a chance to learn the various Linux APIs nor have I been able to attract others to help code review and maintain the project.

If you're interested in helping make some smaller incremental improvements, I'd certainly be grateful. Right now I'm trying to focus on cleaning up the code (see 1f65e2e) so that lower-level concerns are more isolated.

I'm also trying to get an fslog project going to help better understand the behaviour of each OS.

Thanks for all your work. Nathan.

chamaken added 6 commits September 11, 2014 17:08

inotify: add isRunning flag for test

c318f35

inotify: add method for test

1e5070e

to checks internal maps len

test: add for inotify

cc86312

fail in checking maps len

inotify: fix maps deletion

94daf90

inotify(7): IN_IGNORED Watch was removed explicitly (inotify_rm_watch(2)) or automatically (file was deleted, or file system was unmounted). watches is removed in ignoreLinux() and paths too. introduce condv to sync Remove() with ignoreLinux().

inotify: introduce epoll

aa4940f

to finish go routine.

inotify: add test for Remove()

cc35f83

nathany added linux bug labels Sep 11, 2014

nathany reviewed Sep 11, 2014
View reviewed changes

nathany mentioned this pull request Sep 13, 2014

Cannot remove watch for deleted file #40

Closed

chamaken added 2 commits September 13, 2014 15:51

inotify: stop tepid EOF handling

d6718be

handle as same as another error.

inotify: settle defer

3687df3

chamaken closed this Sep 14, 2014

chamaken reopened this Sep 14, 2014

dmitshur reviewed Sep 15, 2014
View reviewed changes

go: go fmt github.com/chamaken/fsnotify/...

f12960a

Thanks to shurcooL, I was able to grasp I should gofmt.

nathany modified the milestone: v2 API changes Sep 24, 2014

chamaken added 2 commits September 25, 2014 16:52

Merge branch 'master' into inotify

5fd57c7

inotify: not call Remove() in Close()

68845ca

inotify(7) says: When all file descriptors referring to an inotify instance have been closed, the underlying object and its resources are freed for reuse by the kernel; all associated watches are automatically freed.

chamaken force-pushed the inotify branch from 5272385 to 68845ca Compare September 30, 2014 09:41

chamaken added 2 commits October 3, 2014 11:33

inotify: close done channel

064f0cf

I had forgotten.

test: add sort of torture

5092cde

nathany mentioned this pull request Nov 15, 2014

inotify blocks #5

Closed

nathany removed this from the v2 Internals milestone Nov 15, 2014

nathany closed this Nov 15, 2014

nathany mentioned this pull request Dec 8, 2014

exp/inotify: Read hangs, AddWatch, RemoveWatch fail on recreated directories golang/go#2483

Closed

miraclesu mentioned this pull request Jan 25, 2016

fix reader nil pointer hpcloud/tail#73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Inotify #39

Inotify #39

chamaken commented Sep 11, 2014

Uh oh!

nathany Sep 11, 2014

Uh oh!

chamaken Sep 12, 2014

Uh oh!

nathany Sep 13, 2014

Uh oh!

chamaken Sep 13, 2014

Uh oh!

nathany commented Sep 11, 2014

Uh oh!

nathany commented Sep 13, 2014

Uh oh!

chamaken commented Sep 14, 2014

Uh oh!

nathany commented Sep 14, 2014

Uh oh!

chamaken commented Sep 14, 2014

Uh oh!

dmitshur Sep 15, 2014

Uh oh!

nathany Sep 15, 2014

Uh oh!

nathany commented Sep 15, 2014

Uh oh!

nathany commented Sep 24, 2014

Uh oh!

chamaken commented Sep 25, 2014

Uh oh!

nathany commented Nov 15, 2014

Uh oh!

		}
		event = &syscall.EpollEvent{syscall.EPOLLIN, int32(rp.Fd()), 0}

Inotify #39

Inotify #39

Conversation

chamaken commented Sep 11, 2014

Uh oh!

nathany Sep 11, 2014

Choose a reason for hiding this comment

Uh oh!

chamaken Sep 12, 2014

Choose a reason for hiding this comment

Uh oh!

nathany Sep 13, 2014

Choose a reason for hiding this comment

Uh oh!

chamaken Sep 13, 2014

Choose a reason for hiding this comment

Uh oh!

nathany commented Sep 11, 2014

Uh oh!

nathany commented Sep 13, 2014

Uh oh!

chamaken commented Sep 14, 2014

Uh oh!

nathany commented Sep 14, 2014

Uh oh!

chamaken commented Sep 14, 2014

Uh oh!

dmitshur Sep 15, 2014

Choose a reason for hiding this comment

Uh oh!

nathany Sep 15, 2014

Choose a reason for hiding this comment

Uh oh!

nathany commented Sep 15, 2014

Uh oh!

nathany commented Sep 24, 2014

Uh oh!

chamaken commented Sep 25, 2014

Uh oh!

nathany commented Nov 15, 2014

Uh oh!