[PATCH 2/2] fake_inotify + kqnotify: rewrite and combine code

[Eric Wong <e@80x24.org>]

KQNotify is now a subclass of FakeInotify since they're both
faking a subset of inotify; and both require directory scanning
via readdir() to detect new/deleted files.

ctime is no longer used with per-file stat to detect new files
with kevent.  That proved too unreliable either due to low
time resolution of the NetBSD/OpenBSD VFS and/or
Time::HiRes::stat being constrained by floating point to
represent `struct timespec', so instead we fuzz the time a bit
if the ctime is recent and merely compare filenames off readdir.

This fixes t/fake_inotify.t and t/kqnotify.t failures under NetBSD
and also removes workarounds for OpenBSD in t/kqnotify.t.  It
also allows us to to remove delays in tests by being more
aggressive in picking up new/deleted files in watch directories
by adjusting the time to scan if the ctime is recent.

This ought to may improve real-world reliability on all *BSDs
regardless of whether IO::KQueue is installed.
---
 It was a total PITA to develop and test this patch
 across 3 BSDs with and without IO::KQueue installed.

 And still other *BSD problems with -watch in t/imapd*.t ...

 lib/PublicInbox/FakeInotify.pm | 186 ++++++++++++++++++---------------
 lib/PublicInbox/KQNotify.pm    |  96 +++++------------
 t/dir_idle.t                   |   5 +-
 t/fake_inotify.t               |  27 +++--
 t/kqnotify.t                   |  23 ----
 5 files changed, 146 insertions(+), 191 deletions(-)

diff --git a/lib/PublicInbox/FakeInotify.pm b/lib/PublicInbox/FakeInotify.pm
index 45b80f50..3a09b030 100644
--- a/lib/PublicInbox/FakeInotify.pm
+++ b/lib/PublicInbox/FakeInotify.pm
@@ -2,12 +2,12 @@
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 
 # for systems lacking Linux::Inotify2 or IO::KQueue, just emulates
-# enough of Linux::Inotify2
+# enough of Linux::Inotify2 we use.
 package PublicInbox::FakeInotify;
 use v5.12;
-use parent qw(Exporter);
 use Time::HiRes qw(stat);
 use PublicInbox::DS qw(add_timer);
+use Errno qw(ENOTDIR ENOENT);
 sub IN_MODIFY () { 0x02 } # match Linux inotify
 # my $IN_MOVED_FROM	 0x00000040	/* File was moved from X.  */
 # my $IN_MOVED_TO = 0x80;
@@ -17,98 +17,124 @@ sub IN_DELETE () { 0x200 }
 sub IN_DELETE_SELF () { 0x400 }
 sub IN_MOVE_SELF () { 0x800 }
 
-our @EXPORT_OK = qw(fill_dirlist on_dir_change);
-
 my $poll_intvl = 2; # same as Filesys::Notify::Simple
 
-sub new { bless { watch => {}, dirlist => {} }, __PACKAGE__ }
+sub new { bless {}, __PACKAGE__ }
 
-sub fill_dirlist ($$$) {
-	my ($self, $path, $dh) = @_;
-	my $dirlist = $self->{dirlist}->{$path} = {};
-	while (defined(my $n = readdir($dh))) {
-		$dirlist->{$n} = undef if $n !~ /\A\.\.?\z/;
-	}
-}
+sub on_dir_change ($$$$$) { # used by KQNotify subclass
+	my ($self, $events, $dh, $path, $dir_delete) = @_;
+	my $old = $self->{dirlist}->{$path};
+	my @cur = grep(!/\A\.\.?\z/, readdir($dh));
+	$self->{dirlist}->{$path} = \@cur;
 
-# behaves like Linux::Inotify2->watch
-sub watch {
-	my ($self, $path, $mask) = @_;
-	my @st = stat($path) or return;
-	my $k = "$path\0$mask";
-	$self->{watch}->{$k} = $st[10]; # 10 - ctime
-	if ($mask & IN_DELETE) {
-		opendir(my $dh, $path) or return;
-		fill_dirlist($self, $path, $dh);
+	# new files:
+	my %tmp = map { $_ => undef } @cur;
+	delete @tmp{@$old};
+	push(@$events, map {
+		bless \"$path/$_", 'PublicInbox::FakeInotify::Event'
+	} keys %tmp);
+
+	if ($dir_delete) {
+		%tmp = map { $_ => undef } @$old;
+		delete @tmp{@cur};
+		push(@$events, map {
+			bless \"$path/$_", 'PublicInbox::FakeInotify::GoneEvent'
+		} keys %tmp);
 	}
-	bless [ $self->{watch}, $k ], 'PublicInbox::FakeInotify::Watch';
 }
 
-# also used by KQNotify since it kevent requires readdir on st_nlink
-# count changes.
-sub on_dir_change ($$$$$) {
-	my ($events, $dh, $path, $old_ctime, $dirlist) = @_;
-	my $oldlist = $dirlist->{$path};
-	my $newlist = $oldlist ? {} : undef;
-	while (defined(my $base = readdir($dh))) {
-		next if $base =~ /\A\.\.?\z/;
-		my $full = "$path/$base";
-		my @st = stat($full);
-		if (@st && $st[10] > $old_ctime) {
-			push @$events,
-				bless(\$full, 'PublicInbox::FakeInotify::Event')
+sub watch_open ($$$) { # used by KQNotify subclass
+	my ($self, $path, $dir_delete) = @_;
+	my ($fh, @st, @st0, $tries);
+	do {
+again:
+		unless (@st0 = stat($path)) {
+			warn "W: stat($path): $!" if $! != ENOENT;
+			return;
 		}
-		if (!@st) {
-			# ignore ENOENT due to race
-			warn "unhandled stat($full) error: $!\n" if !$!{ENOENT};
-		} elsif ($newlist) {
-			$newlist->{$base} = undef;
+		if (!(-d _ ? opendir($fh, $path) : open($fh, '<', $path))) {
+			goto again if $! == ENOTDIR && ++$tries < 10;
+			warn "W: open($path): $!" if $! != ENOENT;
+			return;
 		}
+		@st = stat($fh) or die "fstat($path): $!";
+	} while ("@st[0,1]" ne "@st0[0,1]" &&
+		((++$tries < 10) || (warn(<<EOM) && return)));
+E: $path switching inodes too frequently to watch
+EOM
+	if (-d _) {
+		$self->{dirlist}->{$path} = [];
+		on_dir_change($self, [], $fh, $path, $$dir_delete);
+	} else {
+		$$dir_delete = 0;
 	}
-	return if !$newlist;
-	delete @$oldlist{keys %$newlist};
-	$dirlist->{$path} = $newlist;
-	push(@$events, map {
-		bless \"$path/$_", 'PublicInbox::FakeInotify::GoneEvent'
-	} keys %$oldlist);
+	bless [ @st[0, 1, 10], $path, $fh ], 'PublicInbox::FakeInotify::Watch'
+}
+
+# behaves like Linux::Inotify2->watch
+sub watch {
+	my ($self, $path, $mask) = @_; # mask is ignored
+	my $dir_delete = $mask & IN_DELETE ? 1 : 0;
+	my $w = watch_open($self, $path, \$dir_delete) or return;
+	pop @$w; # no need to keep $fh open for non-kqueue
+	$self->{watch}->{"$path\0$dir_delete"} = $w;
+}
+
+sub gone ($$$) { # used by KQNotify subclass
+	my ($self, $ident, $path) = @_;
+	delete $self->{watch}->{$ident};
+	delete $self->{dirlist}->{$path};
+	bless(\$path, 'PublicInbox::FakeInotify::SelfGoneEvent');
+}
+
+# fuzz the time for freshly modified directories for low-res VFS
+sub dir_adj ($) {
+	my ($old_ctime) = @_;
+	my $now = Time::HiRes::time;
+	my $diff = $now - $old_ctime;
+	($diff > -1 && $diff < 1) ? 1 : 0;
 }
 
 # behaves like non-blocking Linux::Inotify2->read
 sub read {
 	my ($self) = @_;
-	my $watch = $self->{watch} or return ();
-	my $events = [];
-	my @watch_gone;
-	for my $x (keys %$watch) {
-		my ($path, $mask) = split(/\0/, $x, 2);
-		my @now = stat($path);
-		if (!@now && $!{ENOENT} && ($mask & IN_DELETE_SELF)) {
-			push @$events, bless(\$path,
-				'PublicInbox::FakeInotify::SelfGoneEvent');
-			push @watch_gone, $x;
-			delete $self->{dirlist}->{$path};
+	my $ret = [];
+	while (my ($ident, $w) = each(%{$self->{watch}})) {
+		if (!@$w) { # cancelled
+			delete($self->{watch}->{$ident});
+			next;
 		}
-		next if !@now;
-		my $old_ctime = $watch->{$x};
-		$watch->{$x} = $now[10];
-		next if $old_ctime == $now[10];
-		if ($mask & IN_MODIFY) {
-			push @$events,
-				bless(\$path, 'PublicInbox::FakeInotify::Event')
-		} elsif ($mask & (MOVED_TO_OR_CREATE | IN_DELETE)) {
-			if (opendir(my $dh, $path)) {
-				on_dir_change($events, $dh, $path, $old_ctime,
-						$self->{dirlist});
-			} elsif ($!{ENOENT}) {
-				push @watch_gone, $x;
-				delete $self->{dirlist}->{$path};
-			} else {
-				warn "W: opendir $path: $!\n";
+		my $dir_delete = (split(/\0/, $ident, 2))[1];
+		my ($old_dev, $old_ino, $old_ctime, $path) = @$w;
+		my @new_st = stat($path);
+		warn "W: stat($path): $!\n" if !@new_st && $! != ENOENT;
+		if (!@new_st || "$old_dev $old_ino" ne "@new_st[0,1]") {
+			push @$ret, gone($self, $ident, $path);
+			next;
+		}
+		if (-d _ && $new_st[10] > ($old_ctime - dir_adj($old_ctime))) {
+			opendir(my $fh, $path) or do {
+				if ($! == ENOENT || $! == ENOTDIR) {
+					push @$ret, gone($self, $ident, $path);
+				} else {
+					warn "W: opendir($path): $!";
+				}
+				next;
+			};
+			@new_st = stat($fh) or die "fstat($path): $!";
+			if ("$old_dev $old_ino" ne "@new_st[0,1]") {
+				push @$ret, gone($self, $ident, $path);
+				next;
 			}
+			$w->[2] = $new_st[10];
+			on_dir_change($self, $ret, $fh, $path, $dir_delete);
+		} elsif ($new_st[10] > $old_ctime) { # regular files, etc
+			$w->[2] = $new_st[10];
+			push @$ret, bless(\$path,
+					'PublicInbox::FakeInotify::Event');
 		}
 	}
-	delete @$watch{@watch_gone};
-	@$events;
+	@$ret;
 }
 
 sub poll_once {
@@ -120,15 +146,9 @@ sub poll_once {
 package PublicInbox::FakeInotify::Watch;
 use v5.12;
 
-sub cancel {
-	my ($self) = @_;
-	delete $self->[0]->{$self->[1]};
-}
+sub cancel { @{$_[0]} = () }
 
-sub name {
-	my ($self) = @_;
-	(split(/\0/, $self->[1], 2))[0];
-}
+sub name { $_[0]->[3] }
 
 package PublicInbox::FakeInotify::Event;
 use v5.12;
diff --git a/lib/PublicInbox/KQNotify.pm b/lib/PublicInbox/KQNotify.pm
index 381711fa..2efa887d 100644
--- a/lib/PublicInbox/KQNotify.pm
+++ b/lib/PublicInbox/KQNotify.pm
@@ -5,46 +5,31 @@
 # using IO::KQueue on *BSD systems.
 package PublicInbox::KQNotify;
 use v5.12;
+use parent qw(PublicInbox::FakeInotify);
 use IO::KQueue;
 use PublicInbox::DSKQXS; # wraps IO::KQueue for fork-safe DESTROY
-use PublicInbox::FakeInotify qw(fill_dirlist on_dir_change);
-use Time::HiRes qw(stat);
+use Errno qw(ENOENT);
 
 # NOTE_EXTEND detects rename(2), NOTE_WRITE detects link(2)
 sub MOVED_TO_OR_CREATE () { NOTE_EXTEND|NOTE_WRITE }
 
 sub new {
 	my ($class) = @_;
-	bless { dskq => PublicInbox::DSKQXS->new, watch => {} }, $class;
+	bless { dskq => PublicInbox::DSKQXS->new }, $class;
 }
 
 sub watch {
 	my ($self, $path, $mask) = @_;
-	my ($fh, $watch);
-	if (-d $path) {
-		opendir($fh, $path) or return;
-		my @st = stat($fh);
-		$watch = bless [ $fh, $path, $st[10] ],
-			'PublicInbox::KQNotify::Watchdir';
-	} else {
-		open($fh, '<', $path) or return;
-		$watch = bless [ $fh, $path ], 'PublicInbox::KQNotify::Watch';
-	}
-	my $ident = fileno($fh);
+	my $dir_delete = $mask & NOTE_DELETE ? 1 : 0;
+	my $w = $self->watch_open($path, \$dir_delete) or return;
+	$w->[2] = pop @$w; # ctime is unused by this subclass
+	my $ident = fileno($w->[2]) // die "BUG: bad fileno $w->[2]: $!";
 	$self->{dskq}->{kq}->EV_SET($ident, # ident (fd)
 		EVFILT_VNODE, # filter
 		EV_ADD | EV_CLEAR, # flags
 		$mask, # fflags
-		0, 0); # data, udata
-	if ($mask & (MOVED_TO_OR_CREATE|NOTE_DELETE|NOTE_LINK|NOTE_REVOKE)) {
-		$self->{watch}->{$ident} = $watch;
-		if ($mask & (NOTE_DELETE|NOTE_LINK|NOTE_REVOKE)) {
-			fill_dirlist($self, $path, $fh)
-		}
-	} else {
-		die "TODO Not implemented: $mask";
-	}
-	$watch;
+		0, $dir_delete); # data, udata
+	$self->{watch}->{$ident} = $w;
 }
 
 # emulate Linux::Inotify::fileno
@@ -61,54 +46,31 @@ sub blocking {}
 # behave like Linux::Inotify2->read
 sub read {
 	my ($self) = @_;
-	my @kevents = $self->{dskq}->{kq}->kevent(0);
 	my $events = [];
-	my @gone;
-	my $watch = $self->{watch};
-	for my $kev (@kevents) {
+	for my $kev ($self->{dskq}->{kq}->kevent(0)) {
 		my $ident = $kev->[KQ_IDENT];
-		my $mask = $kev->[KQ_FFLAGS];
-		my ($dh, $path, $old_ctime) = @{$watch->{$ident}};
-		if (!defined($old_ctime)) {
-			push @$events,
-				bless(\$path, 'PublicInbox::FakeInotify::Event')
-		} elsif ($mask & (MOVED_TO_OR_CREATE|NOTE_DELETE|NOTE_LINK|
-				NOTE_REVOKE|NOTE_RENAME)) {
-			my @new_st = stat($path);
-			if (!@new_st && $!{ENOENT}) {
-				push @$events, bless(\$path,
-						'PublicInbox::FakeInotify::'.
-						'SelfGoneEvent');
-				push @gone, $ident;
-				delete $self->{dirlist}->{$path};
-				next;
-			}
-			if (!@new_st) {
-				warn "unhandled stat($path) error: $!\n";
-				next;
-			}
-			$watch->{$ident}->[3] = $new_st[10]; # ctime
-			rewinddir($dh);
-			on_dir_change($events, $dh, $path, $old_ctime,
-					$self->{dirlist});
+		my $w = $self->{watch}->{$ident} or next;
+		if (!@$w) { # cancelled
+			delete($self->{watch}->{$ident});
+			next;
+		}
+		my $dir_delete = $kev->[KQ_UDATA];
+		my ($old_dev, $old_ino, $fh, $path) = @$w;
+		my @new_st = stat($path);
+		warn "W: stat($path): $!\n" if !@new_st && $! != ENOENT;
+		if (!@new_st || "$old_dev $old_ino" ne "@new_st[0,1]") {
+			push(@$events, $self->gone($ident, $path));
+			next;
+		}
+		if (-d _) {
+			rewinddir($fh);
+			$self->on_dir_change($events, $fh, $path, $dir_delete);
+		} else {
+			push @$events, bless(\$path,
+					'PublicInbox::FakeInotify::Event');
 		}
 	}
-	delete @$watch{@gone};
 	@$events;
 }
 
-package PublicInbox::KQNotify::Watch;
-use v5.12;
-
-sub name { $_[0]->[1] }
-
-sub cancel { close $_[0]->[0] or die "close: $!" }
-
-package PublicInbox::KQNotify::Watchdir;
-use v5.12;
-
-sub name { $_[0]->[1] }
-
-sub cancel { closedir $_[0]->[0] or die "closedir: $!" }
-
 1;
diff --git a/t/dir_idle.t b/t/dir_idle.t
index 50e1dd27..9e66cc58 100644
--- a/t/dir_idle.t
+++ b/t/dir_idle.t
@@ -14,14 +14,12 @@ $di->add_watches(["$tmpdir/a", "$tmpdir/c"], 1);
 PublicInbox::DS->SetLoopTimeout(1000);
 my $end = 3 + now;
 local @PublicInbox::DS::post_loop_do = (sub { scalar(@x) == 0 && now < $end });
-tick(0.011);
 rmdir("$tmpdir/a/b") or xbail "rmdir $!";
 PublicInbox::DS::event_loop();
-is(scalar(@x), 1, 'got an event') and
+is(scalar(@x), 1, 'got an rmdir event') and
 	is($x[0]->[0]->fullname, "$tmpdir/a/b", 'got expected fullname') and
 	ok($x[0]->[0]->IN_DELETE, 'IN_DELETE set');
 
-tick(0.011);
 rmdir("$tmpdir/a") or xbail "rmdir $!";
 @x = ();
 $end = 3 + now;
@@ -30,7 +28,6 @@ is(scalar(@x), 1, 'got an event') and
 	is($x[0]->[0]->fullname, "$tmpdir/a", 'got expected fullname') and
 	ok($x[0]->[0]->IN_DELETE_SELF, 'IN_DELETE_SELF set');
 
-tick(0.011);
 rename("$tmpdir/c", "$tmpdir/j") or xbail "rmdir $!";
 @x = ();
 $end = 3 + now;
diff --git a/t/fake_inotify.t b/t/fake_inotify.t
index 734ddbfb..56f64588 100644
--- a/t/fake_inotify.t
+++ b/t/fake_inotify.t
@@ -1,13 +1,12 @@
 #!perl -w
-# Copyright (C) 2020-2021 all contributors <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 #
 # Ensure FakeInotify can pick up rename(2) and link(2) operations
 # used by Maildir writing tools
-use strict;
+use v5.12;
 use PublicInbox::TestCommon;
 use_ok 'PublicInbox::FakeInotify';
-my $MIN_FS_TICK = 0.011; # for low-res CONFIG_HZ=100 systems
 my ($tmpdir, $for_destroy) = tmpdir();
 mkdir "$tmpdir/new" or BAIL_OUT "mkdir: $!";
 mkdir "$tmpdir/new/rmd" or BAIL_OUT "mkdir: $!";
@@ -18,35 +17,35 @@ my $fi = PublicInbox::FakeInotify->new;
 my $mask = PublicInbox::FakeInotify::MOVED_TO_OR_CREATE();
 my $w = $fi->watch("$tmpdir/new", $mask);
 
-tick $MIN_FS_TICK;
 rename("$tmpdir/tst", "$tmpdir/new/tst") or BAIL_OUT "rename: $!";
 my @events = map { $_->fullname } $fi->read;
-is_deeply(\@events, ["$tmpdir/new/tst"], 'rename(2) detected');
+is_deeply(\@events, ["$tmpdir/new/tst"], 'rename(2) detected') or
+	diag explain(\@events);
 
-tick $MIN_FS_TICK;
 open $fh, '>', "$tmpdir/tst" or BAIL_OUT "open: $!";
 close $fh or BAIL_OUT "close: $!";
 link("$tmpdir/tst", "$tmpdir/new/link") or BAIL_OUT "link: $!";
 @events = map { $_->fullname } $fi->read;
-is_deeply(\@events, ["$tmpdir/new/link"], 'link(2) detected');
+is_deeply(\@events, ["$tmpdir/new/link"], 'link(2) detected') or
+	diag explain(\@events);
 
 $w->cancel;
-tick $MIN_FS_TICK;
 link("$tmpdir/new/tst", "$tmpdir/new/link2") or BAIL_OUT "link: $!";
 @events = map { $_->fullname } $fi->read;
-is_deeply(\@events, [], 'link(2) not detected after cancel');
+is_deeply(\@events, [], 'link(2) not detected after cancel') or
+	diag explain(\@events);
 $fi->watch("$tmpdir/new", PublicInbox::FakeInotify::IN_DELETE());
 
-tick $MIN_FS_TICK;
 rmdir("$tmpdir/new/rmd") or xbail "rmdir: $!";
 @events = $fi->read;
-is_deeply([map{ $_->fullname }@events], ["$tmpdir/new/rmd"], 'rmdir detected');
-ok($events[0]->IN_DELETE, 'IN_DELETE set on rmdir');
+is_deeply([map{ $_->fullname }@events], ["$tmpdir/new/rmd"], 'rmdir detected') or
+	diag explain(\@events);
+ok($events[-1]->IN_DELETE, 'IN_DELETE set on rmdir');
 
-tick $MIN_FS_TICK;
 unlink("$tmpdir/new/tst") or xbail "unlink: $!";
 @events = grep { ref =~ /Gone/ } $fi->read;
-is_deeply([map{ $_->fullname }@events], ["$tmpdir/new/tst"], 'unlink detected');
+is_deeply([map{ $_->fullname }@events], ["$tmpdir/new/tst"], 'unlink detected') or
+	diag explain(\@events);
 ok($events[0]->IN_DELETE, 'IN_DELETE set on unlink');
 
 PublicInbox::DS->Reset;
diff --git a/t/kqnotify.t b/t/kqnotify.t
index 21381806..edecf2e1 100644
--- a/t/kqnotify.t
+++ b/t/kqnotify.t
@@ -17,41 +17,18 @@ my $kqn = PublicInbox::KQNotify->new;
 my $mask = PublicInbox::KQNotify::MOVED_TO_OR_CREATE();
 my $w = $kqn->watch("$tmpdir/new", $mask);
 
-# Unlike FreeBSD, OpenBSD (tested 7.3) kevent NOTE_EXTEND doesn't detect
-# renames into directories reliably.  It's kevent(3) manpage doesn't
-# document this behavior (unlike FreeBSD), but it sometimes works...
 open my $fh, '>', "$tmpdir/tst";
 close $fh;
 rename("$tmpdir/tst", "$tmpdir/new/tst");
 my $hit = [ map { $_->fullname } $kqn->read ];
-my $try = 0;
-while (!@$hit && $^O eq 'openbsd' && $try++ < 30) {
-	diag "retrying NOTE_EXTEND detection for $^O (#$try)";
-	# kevent can totally ignore the event, so delaying hasn't worked;
-	# keep doing the same thing until kevent notices one of them
-	open $fh, '>', "$tmpdir/tst";
-	close $fh;
-	rename("$tmpdir/tst", "$tmpdir/new/tst");
-	$hit = [ map { $_->fullname } $kqn->read ]
-}
 is_deeply($hit, ["$tmpdir/new/tst"],
 		'rename(2) detected (via NOTE_EXTEND)')
 		or diag explain($hit);
 
-# OpenBSD (tested 7.3) doesn't reliably trigger NOTE_WRITE on link(2)
-# into directories, but usually it does (and more reliably than rename(2)
-# above) and doesn't drop the event entirely.
 open $fh, '>', "$tmpdir/tst";
 close $fh;
 link("$tmpdir/tst", "$tmpdir/new/link");
 my @read = map { $_->fullname } $kqn->read;
-$try = 0;
-while (!@read && $^O eq 'openbsd' && $try++ < 30) {
-	diag "delaying and retrying NOTE_WRITE detection for $^O (#$try)";
-	tick;
-	# no need to link(2) again, at least, kevent can just be late, here
-	@read = map { $_->fullname } $kqn->read;
-}
 $hit = [ grep(m!/link$!, @read) ];
 is_deeply($hit, ["$tmpdir/new/link"], 'link(2) detected (via NOTE_WRITE)')
 	or diag explain(\@read);