Make inotify.Inotify more robust; remove inotify.Watcher. BREAKING CHANGE.

3 files changed, 205 insertions, 175 deletions

diff --git a/inotify/channels.go b/inotify/channels.go
deleted file mode 100644
index 737b312..0000000
--- a/inotify/channels.go
+++ /dev/null
@@ -1,96 +0,0 @@
-// Copyright 2015 Luke Shumaker <lukeshu@sbcglobal.net>.
-//
-// This is free software; you can redistribute it and/or modify it
-// under the terms of the GNU Lesser General Public License as
-// published by the Free Software Foundation; either version 2.1 of
-// the License, or (at your option) any later version.
-//
-// This software is distributed in the hope that it will be useful,
-// but WITHOUT ANY WARRANTY; without even the implied warranty of
-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU Lesser General Public License for more details.
-//
-// You should have received a copy of the GNU Lesser General Public
-// License along with this manual; if not, see
-// <http://www.gnu.org/licenses/>.
-
-package inotify
-
-import (
-	"os"
-	"syscall"
-)
-
-// A Watcher is a wrapper around an (*Inotify) that exposes a
-// channel-based interface that is much nicer to work with.
-type Watcher struct {
-	Events <-chan Event
-	Errors <-chan error
-	events chan<- Event
-	errors chan<- error
-	in     *Inotify
-}
-
-// Wraps InotifyInit()
-func WatcherInit() (*Watcher, error) {
-	in, err := InotifyInit()
-	return newWatcher(in, err)
-}
-
-// Wraps InotifyInit1()
-func WatcherInit1(flags int) (*Watcher, error) {
-	in, err := InotifyInit1(flags &^ IN_NONBLOCK)
-	return newWatcher(in, err)
-}
-
-func newWatcher(in *Inotify, err error) (*Watcher, error) {
-	events := make(chan Event)
-	errors := make(chan error)
-	o := &Watcher{
-		Events: events,
-		events: events,
-		Errors: errors,
-		errors: errors,
-		in:     in,
-	}
-	go o.worker()
-	return o, err
-}
-
-// Wraps Inotify.AddWatch(); adds or modifies a watch.
-func (o *Watcher) AddWatch(path string, mask Mask) (Wd, error) {
-	return o.in.AddWatch(path, mask)
-}
-
-// Wraps Inotify.RmWatch(); removes a watch.
-func (o *Watcher) RmWatch(wd Wd) error {
-	return o.in.RmWatch(wd)
-}
-
-// Wraps Inotify.Close().  Unlike Inotify.Close(),
-// this cannot block.  Also unlike Inotify.Close(), nothing
-// may be received from the channel after this is called.
-func (o *Watcher) Close() {
-	func() {
-		defer recover()
-		close(o.events)
-		close(o.errors)
-	}()
-	go o.in.Close()
-}
-
-func (o *Watcher) worker() {
-	defer recover()
-	for {
-		ev, err := o.in.Read()
-		if ev.Wd >= 0 {
-			o.events <- ev
-		}
-		if err != nil {
-			if err.(*os.SyscallError).Err == syscall.EBADF {
-				o.Close()
-			}
-			o.errors <- err
-		}
-	}
-}
diff --git a/inotify/inotify.go b/inotify/inotify.go
index 2fd3a83..2db414c 100644
--- a/inotify/inotify.go
+++ b/inotify/inotify.go
@@ -1,4 +1,4 @@
-// Copyright 2015 Luke Shumaker <lukeshu@sbcglobal.net>.
+// Copyright 2015-2016 Luke Shumaker <lukeshu@sbcglobal.net>.
 //
 // This is free software; you can redistribute it and/or modify it
 // under the terms of the GNU Lesser General Public License as
@@ -22,16 +22,54 @@ import (
 	"sync"
 	"syscall"
 	"unsafe"
+	"os"
 )
 
+// Most of the complexity here is that we syncronize around access to
+// the file descriptor.  Why?  Because we're worried that it could get
+// closed and re-used between the time the fd is read from the os.File
+// and the time the system call is actually dispatched.
+//
+//              A                        B
+//         in.method()       in.Close(); syscall.Open()
+//     ---------------------------------------------------
+//     fd = in.fd
+//                                syscall.Close()
+//                                syscall.Open()
+//     syscall(fd)
+//
+// And you're wondering "should we really protoect against that; after
+// all: share by communicating, don't communicate by sharing" and "why
+// do we have to deal with this condition if os.File doesn't?"
+// Because an inotify instance is more like a net.Listener.  If you
+// notice, net.netFD has do deal with a very similar situtation.  At
+// some level, you may simply observe that with inotify it makes sense
+// to have one goroutine add/remove watches, and have another read
+// them; where this kind of communication doesn't make sense with
+// ordinary files.
+//
+// So, how does net.netFD deal with it?  Well... it is tightly coupled
+// with some private routines in runtime/sema.go.  That is, we can't
+// look at it for too much guidance.
+
 type Inotify struct {
-	fd       file
-	fdLock   sync.RWMutex
-	buffFull [4096]byte
+	fd      inFd
+	fdLock  sync.RWMutex
+	fdBlock bool
+
+	buffFull [4096]byte // 4KiB is a good size, but the bare
+			    // minimum is `sizeof(struct
+			    // inotify_event) + NAME_MAX + 1`
 	buff     []byte
 	buffLock sync.Mutex
+	buffErr  error
+
+	ch chan chev
 }
 
+// Event is a file system event from inotify.
+//
+// An Event is invalid if Wd is < 0.
 type Event struct {
 	Wd     Wd      // Watch descriptor
 	Mask   Mask    // Mask describing event
@@ -39,90 +77,178 @@ type Event struct {
 	Name   *string // Optional name
 }
 
-// Create an inotify instance.  The variant InotifyInit1() allows
-// flags to access extra functionality.
-func InotifyInit() (*Inotify, error) {
-	fd, err := inotify_init()
-	o := Inotify{
+type chev struct {
+	Ev  Event
+	Err error
+}
+
+func newInotify(fd inFd, blocking bool) *Inotify {
+	if fd < 0 {
+		return nil
+	}
+	in := &Inotify{
 		fd: fd,
+		fdBlock: blocking,
+		ch: make(chan chev),
 	}
-	o.buff = o.buffFull[:0]
-	return &o, err
+	in.buff = in.buffFull[:0]
+	go in.worker()
+	return in
 }
 
-// Create an inotify instance, with flags specifying extra
-// functionality.
+// InotifyInit creates an inotify instance.  The variant
+// InotifyInit1() allows flags to access extra functionality.
+func InotifyInit() (*Inotify, error) {
+	fd, err := sys_inotify_init()
+	return newInotify(fd, true), err
+}
+
+// InotifyInit1 create an inotify instance, with flags specifying
+// extra functionality.
 func InotifyInit1(flags int) (*Inotify, error) {
-	fd, err := inotify_init1(flags)
-	o := Inotify{
-		fd: fd,
-	}
-	o.buff = o.buffFull[:0]
-	return &o, err
+	fd, err := sys_inotify_init1(flags &^ IN_NONBLOCK)
+	return newInotify(fd, flags & IN_NONBLOCK == 0), err
 }
 
-// Add a watch to the inotify instance, or modifies an existing watch
-// item.
-func (o *Inotify) AddWatch(path string, mask Mask) (Wd, error) {
-	o.fdLock.RLock()
-	defer o.fdLock.RUnlock()
-	return inotify_add_watch(o.fd, path, mask)
+// AddWatch adds a watch to the inotify instance, or modifies an
+// existing watch item.
+func (in *Inotify) AddWatch(path string, mask Mask) (Wd, error) {
+	in.fdLock.RLock()
+	defer in.fdLock.RUnlock()
+	return sys_inotify_add_watch(in.fd, path, mask)
 }
 
-// Remove a watch from the inotify instance.
-func (o *Inotify) RmWatch(wd Wd) error {
-	o.fdLock.RLock()
-	defer o.fdLock.RUnlock()
-	return inotify_rm_watch(o.fd, wd)
+// RmWatch removes a watch from the inotify instance.
+func (in *Inotify) RmWatch(wd Wd) error {
+	in.fdLock.RLock()
+	defer in.fdLock.RUnlock()
+	return sys_inotify_rm_watch(in.fd, wd)
 }
 
-// Close the inotify instance; further calls to this object will
-// error.
-//
-// Events recieved before Close() is called may still be Read() after
-// the call to Close().
-//
-// Beware that if Close() is called while waiting on Read(), it will
-// block until events are read.
-func (o *Inotify) Close() error {
-	o.fdLock.Lock()
-	defer o.fdLock.Unlock()
-	defer func() { o.fd = -1 }()
-	return sysclose(o.fd)
+// Close closes the inotify instance; further calls to this object
+// will error.
+func (in *Inotify) Close() (err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			// This is a double-close condition.
+			//
+			// Choices for the error:
+			// - Linux: EBADF
+			// - os.File: syscall.EINVAL
+			// - net.netFD: net.errClosing = errors.New(...)
+			err = os.NewSyscallError("close", syscall.EBADF)
+		}
+	}()
+	close(in.ch) // will panic if already closed; hence above
+
+	// I would love to be able to return the error from sys_close;
+	// but it might block forever.
+	go func() {
+		in.fdLock.Lock()
+		in.fdLock.Unlock()
+		sys_close(in.fd)
+	}()
+	return
 }
 
-// Read an event from the inotify instance.
+// read is a low-level read an event from the inotify instance.
 //
-// Events recieved before Close() is called may still be Read() after
-// the call to Close().
-func (o *Inotify) Read() (Event, error) {
-	o.buffLock.Lock()
-	defer o.buffLock.Unlock()
-
-	if len(o.buff) == 0 {
-		o.fdLock.RLock()
-		len, err := sysread(o.fd, o.buffFull[:])
-		o.fdLock.RUnlock()
-		if len == 0 {
-			return Event{Wd: -1}, o.Close()
-		} else if len < 0 {
-			return Event{Wd: -1}, err
+// It's low-level/private because it can deadlock between it and the
+// `go` block in Close.  Instead, a worker calls this in a loop,
+// writes the result to a channel, and a public reader can safely get
+// it from the channel.  No deadlock.
+func (in *Inotify) read() (Event, error) {
+	in.buffLock.Lock()
+	defer in.buffLock.Unlock()
+
+	if len(in.buff) == 0 {
+		if in.buffErr != nil {
+			return Event{Wd: -1}, in.buffErr
 		}
-		o.buff = o.buffFull[0:len]
+		var n int
+		in.fdLock.RLock()
+		n, in.buffErr = sys_read(in.fd, in.buffFull[:])
+		in.fdLock.RUnlock()
+		in.buff = in.buffFull[0:n]
 	}
 
-	raw := (*syscall.InotifyEvent)(unsafe.Pointer(&o.buff[0]))
+	if len(in.buff) < syscall.SizeofInotifyEvent {
+		// Either Linux screwed up (and we have no chance of
+		// handling that sanely), or this Inotify came from an
+		// existing FD that wasn't really an inotify instance.
+		in.buffErr = syscall.EBADF
+		return Event{Wd: -1}, in.buffErr
+	}
+	raw := (*syscall.InotifyEvent)(unsafe.Pointer(&in.buff[0]))
 	ret := Event{
 		Wd:     Wd(raw.Wd),
 		Mask:   Mask(raw.Mask),
 		Cookie: raw.Cookie,
 		Name:   nil,
 	}
+	if int64(len(in.buff)) < syscall.SizeofInotifyEvent+int64(raw.Len) {
+		// Same as above.
+		in.buffErr = syscall.EBADF
+		return Event{Wd: -1}, in.buffErr
+	}
 	if raw.Len > 0 {
-		bytes := (*[syscall.NAME_MAX]byte)(unsafe.Pointer(&o.buff[syscall.SizeofInotifyEvent]))
+		bytes := (*[syscall.NAME_MAX]byte)(unsafe.Pointer(&in.buff[syscall.SizeofInotifyEvent]))
 		name := string(bytes[:raw.Len-1])
 		ret.Name = &name
 	}
-	o.buff = o.buff[0 : syscall.SizeofInotifyEvent+raw.Len]
+	in.buff = in.buff[0 : syscall.SizeofInotifyEvent+raw.Len]
 	return ret, nil
 }
+
+func (in *Inotify) worker() {
+	defer recover()
+	for {
+		ev, err := in.read()
+		in.ch <- chev{ev, err} // will panic on .Close()
+	}
+}
+
+// Read calls either ReadBlock or ReadNonblock, depending on whether
+// the Inotify instance has the IN_NONBLOCK flag set.
+func (in *Inotify) Read() (Event, error) {
+	if in.fdBlock {
+		return in.ReadBlock()
+	} else {
+		return in.ReadNonblock()
+	}
+}
+
+// ReadBlock reads exactly one Event or an error from the inotify
+// instance.  If an Event or error is not available for reading, it
+// blocks until one is.
+//
+// If err is non-nil, then the Event is invalid (ev.Wd < 0); and if
+// err is nil, then then the Event is valid.
+func (in *Inotify) ReadBlock() (Event, error) {
+	e, ok := <-in.ch
+	if !ok {
+		return Event{Wd: -1}, os.NewSyscallError("read", syscall.EBADF)
+	}
+	return e.Ev, e.Err
+}
+
+// ReadNonblock reads either an Event or an error from the inotify
+// instance, but doesn't block if an event isn't ready.
+//
+// Unlike Read, it may be the case that both the Event is invalid and
+// the error is nil.  This indicates that the read simply returned
+// instead of blocking.
+//
+//     ev, err := in.ReadNonblock()
+//     haveRead = ev.Wd >= 0 || err != nil
+func (in *Inotify) ReadNonblock() (Event, error) {
+	select {
+	case e, ok := <-in.ch:
+		if !ok {
+			return Event{Wd: -1}, os.NewSyscallError("read", syscall.EBADF)
+		}
+		return e.Ev, e.Err
+	default:
+		return Event{Wd: -1}, nil
+	}
+}
diff --git a/inotify/syscall.go b/inotify/syscall.go
index d1b5140..0a0c2f0 100644
--- a/inotify/syscall.go
+++ b/inotify/syscall.go
@@ -1,4 +1,4 @@
-// Copyright 2015 Luke Shumaker <lukeshu@sbcglobal.net>.
+// Copyright 2015-2016 Luke Shumaker <lukeshu@sbcglobal.net>.
 //
 // This is free software; you can redistribute it and/or modify it
 // under the terms of the GNU Lesser General Public License as
@@ -29,35 +29,35 @@ func newPathError(op string, path string, err error) error {
 }
 
 // Create and initialize inotify instance.
-func inotify_init() (file, error) {
-	fd, errno := syscall.InotifyInit()
-	return file(fd), os.NewSyscallError("inotify_init", errno)
+func sys_inotify_init() (inFd, error) {
+	fd, err := syscall.InotifyInit()
+	return inFd(fd), os.NewSyscallError("inotify_init", err)
 }
 
 // Create and initialize inotify instance.
-func inotify_init1(flags int) (file, error) {
-	fd, errno := syscall.InotifyInit1(flags)
-	return file(fd), os.NewSyscallError("inotify_init1", errno)
+func sys_inotify_init1(flags int) (inFd, error) {
+	fd, err := syscall.InotifyInit1(flags)
+	return inFd(fd), os.NewSyscallError("inotify_init1", err)
 }
 
 // Add watch of object NAME to inotify instance FD.  Notify about
 // events specified by MASK.
-func inotify_add_watch(fd file, name string, mask Mask) (Wd, error) {
-	wd, errno := syscall.InotifyAddWatch(int(fd), name, uint32(mask))
-	return Wd(wd), newPathError("inotify_add_watch", name, errno)
+func sys_inotify_add_watch(fd inFd, name string, mask Mask) (Wd, error) {
+	wd, err := syscall.InotifyAddWatch(int(fd), name, uint32(mask))
+	return Wd(wd), newPathError("inotify_add_watch", name, err)
 }
 
 // Remove the watch specified by WD from the inotify instance FD.
-func inotify_rm_watch(fd file, wd Wd) error {
-	success, errno := syscall.InotifyRmWatch(int(fd), uint32(wd))
+func sys_inotify_rm_watch(fd inFd, wd Wd) error {
+	success, err := syscall.InotifyRmWatch(int(fd), uint32(wd))
 	switch success {
 	case -1:
-		if errno == nil {
+		if err == nil {
 			panic("should never happen")
 		}
-		os.NewSyscallError("inotify_rm_watch", errno)
+		os.NewSyscallError("inotify_rm_watch", err)
 	case 0:
-		if errno != nil {
+		if err != nil {
 			panic("should never happen")
 		}
 		return nil
@@ -65,11 +65,11 @@ func inotify_rm_watch(fd file, wd Wd) error {
 	panic("should never happen")
 }
 
-func sysclose(fd file) error {
+func sys_close(fd inFd) error {
 	return os.NewSyscallError("close", syscall.Close(int(fd)))
 }
 
-func sysread(fd file, p []byte) (int, error) {
+func sys_read(fd inFd, p []byte) (int, error) {
 	n, err := syscall.Read(int(fd), p)
 	return n, os.NewSyscallError("read", err)
 }