Move some inotify-tests to run on all backends; test that state is cl…

…eaned up after Remove (#494)

This moves a number of tests from backend_inotify_test.go to
fsnotify_test.go, as they're not really inotify-specific. In particular,
it moves the "stress test", which creates a bunch of events/files. It
also expands this test to (potentially) create many more than just 1,000
files, depending on how many the system will allow.

Unfortunately these tests seem pretty flaky on kqueue platforms, where
they're allowed to fail for now (failing the test won't fail the test
run). This seems to expose some existing limits/problems that need to
fixed in a future PR.

Also test that the internal state is cleaned up with TestRemoveState().
The Windows backend doesn't have a test for it (or rather, it doesn't
run) as it *doesn't* clean the state properly, but I found it too
confusing to fix 🤷 Need to spend some time on that in the future.

Reorder/rename some GitHub Actions test runs to show nicer in the UI.

Fixes #42
Fixes #268

.github/workflows/test.yml

@@ -4,14 +4,13 @@ on:
     paths: ['**.go', 'go.mod', '.github/workflows/*']
 
 jobs:
+  # Test Windows and Linux with the latest Go version and the oldest we support.
   test:
     strategy:
       fail-fast: false
       matrix:
         os:
           - ubuntu-latest
-          - macos-11
-          - macos-12
           - windows-latest
         go:
           - '1.16'
@@ -30,6 +29,35 @@ jobs:
         run: |
           go test -race ./...
 
+  # Test only the latest Go version on macOS; we use the macOS builders for BSD
+  # and illumos, and GitHub doesn't allow many of them to run concurrently. If
+  # it works on Windows and Linux with Go 1.16, then it probably does on macOS
+  # too.
+  testMacOS:
+    name: test
+    strategy:
+      fail-fast: false
+      matrix:
+        os:
+          - macos-11
+          - macos-12
+        go:
+          - '1.19'
+    runs-on: ${{ matrix.os }}
+    steps:
+      - name: setup Go
+        uses: actions/setup-go@v3
+        with:
+          go-version: ${{ matrix.go }}
+
+      - name: checkout
+        uses: actions/checkout@v3
+
+      - name: test
+        run: |
+          go test -race ./...
+
+  # FreeBSD
   testFreeBSD:
     runs-on: macos-12
     name: test (freebsd, 1.18)
@@ -45,6 +73,13 @@ jobs:
             pw user add -n action -m
             su action -c 'go test -race ./...'
 
+  # OpenBSD; no -race as the VM doesn't include the comp set.
+  #
+  # TODO: should probably add this, but on my local machine the tests time out
+  #       with -race as the waits aren't long enough (OpenBSD is kind of slow),
+  #       so should probably look into that first. Go 1.19 is supposed to have a
+  #       much faster race detector, so maybe waiting until we have that is
+  #       enough.
   testOpenBSD:
     runs-on: macos-12
     name: test (openbsd, 1.17)
@@ -55,34 +90,27 @@ jobs:
         uses: vmactions/openbsd-vm@v0.0.6
         with:
           prepare: pkg_add go
-          # No -race as the VM doesn't include the comp set.
-          #
-          # TODO: should probably add this, but on my local machine the tests
-          #       time out with -race as the waits aren't long enough (OpenBSD
-          #       is kind of slow), so should probably look into that first.
-          #       Go 1.19 is supposed to have a much faster race detector, so
-          #       maybe waiting until we have that is enough.
           run: |
-            # Default of 512 leads to "too many open files".
-            ulimit -n 1024
             useradd -mG wheel action
             su action -c 'go test ./...'
 
+  # NetBSD
   testNetBSD:
     runs-on: macos-12
-    name: test (netbsd, 1.17)
+    name: test (netbsd, 1.18)
     steps:
       - uses: actions/checkout@v3
-      - name: test (netbsd, 1.17)
+      - name: test (netbsd, 1.18)
         id: test
         uses: vmactions/netbsd-vm@v0.0.4
         with:
           prepare: pkg_add go
           # TODO: no -race for the same reason as OpenBSD (the timing; it does run).
           run: |
             useradd -mG wheel action
-            su action -c 'go117 test ./...'
+            su action -c 'go118 test ./...'
 
+  # illumos
   testillumos:
     runs-on: macos-12
     name: test (illumos, 1.17)
@@ -96,3 +124,39 @@ jobs:
           pkg install go-117
         run: |
           /opt/ooce/go-1.17/bin/go test ./...
+
+  # Older Debian 6, for old Linux kernels.
+  testDebian6:
+    runs-on: macos-12
+    name: test (debian6, 1.19)
+    strategy:
+      fail-fast: false
+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Cache Vagrant boxes
+        uses: actions/cache@v3
+        with:
+          path: ~/.vagrant.d/boxes
+          key: ${{ runner.os }}-vagrant-${{ hashFiles('Vagrantfile') }}
+          restore-keys: |
+            ${{ runner.os }}-vagrant-
+
+      - name: setup Go
+        uses: actions/setup-go@v3
+        with:
+          go-version: '1.19'
+
+      - name: test (debian6, 1.19)
+        id: test
+        run: |
+          cp -f .github/workflows/Vagrantfile.debian6 Vagrantfile
+          export GOOS=linux
+          export GOARCH=amd64
+          for p in $(go list ./...); do
+            go test -c -o ${p//\//-}.test $p
+          done
+          vagrant up
+          for t in *.test; do
+            vagrant ssh -c "/vagrant/$t"
+          done

.gitignore

@@ -1,5 +1,6 @@
 # go test -c output
 *.test
+*.test.exe
 
 # Output of go build ./cmd/fsnotify
 /fsnotify

CONTRIBUTING.md

@@ -18,7 +18,8 @@ Just `go test ./...` runs all the tests; the CI runs this on all supported
 platforms. Testing different platforms locally can be done with something like
 [goon] or [Vagrant], but this isn't super-easy to set up at the moment.
 
-The main tests are in [integration_test.go].
+Use the `-short` flag to make the "stress test" run faster.
+
 
 [goon]: https://github.com/arp242/goon
 [Vagrant]: https://www.vagrantup.com/

backend_inotify.go

@@ -72,8 +72,8 @@ func (w *Watcher) sendError(err error) bool {
 	case w.Errors <- err:
 		return true
 	case <-w.done:
+		return false
 	}
-	return false
 }
 
 func (w *Watcher) isClosed() bool {
@@ -97,7 +97,8 @@ func (w *Watcher) Close() error {
 	close(w.done)
 	w.mu.Unlock()
 
-	// Causes any blocking reads to return with an error, provided the file still supports deadline operations
+	// Causes any blocking reads to return with an error, provided the file
+	// still supports deadline operations.
 	err := w.inotifyFile.Close()
 	if err != nil {
 		return err
@@ -170,12 +171,16 @@ func (w *Watcher) Remove(name string) error {
 	// by another thread and we have not received IN_IGNORE event.
 	success, errno := unix.InotifyRmWatch(w.fd, watch.wd)
 	if success == -1 {
-		// TODO: Perhaps it's not helpful to return an error here in every case.
-		// the only two possible errors are:
-		// EBADF, which happens when w.fd is not a valid file descriptor of any kind.
-		// EINVAL, which is when fd is not an inotify descriptor or wd is not a valid watch descriptor.
-		// Watch descriptors are invalidated when they are removed explicitly or implicitly;
-		// explicitly by inotify_rm_watch, implicitly when the file they are watching is deleted.
+		// TODO: Perhaps it's not helpful to return an error here in every case;
+		//       The only two possible errors are:
+		//
+		//       - EBADF, which happens when w.fd is not a valid file descriptor
+		//         of any kind.
+		//       - EINVAL, which is when fd is not an inotify descriptor or wd
+		//         is not a valid watch descriptor. Watch descriptors are
+		//         invalidated when they are removed explicitly or implicitly;
+		//         explicitly by inotify_rm_watch, implicitly when the file they
+		//         are watching is deleted.
 		return errno
 	}
 
@@ -203,15 +208,16 @@ type watch struct {
 // readEvents reads from the inotify file descriptor, converts the
 // received events into Event objects and sends them via the Events channel
 func (w *Watcher) readEvents() {
+	defer func() {
+		close(w.doneResp)
+		close(w.Errors)
+		close(w.Events)
+	}()
+
 	var (
 		buf   [unix.SizeofInotifyEvent * 4096]byte // Buffer for a maximum of 4096 raw events
 		errno error                                // Syscall errno
 	)
-
-	defer close(w.doneResp)
-	defer close(w.Errors)
-	defer close(w.Events)
-
 	for {
 		// See if we have been closed.
 		if w.isClosed() {