[PATCH] notify rewrite;

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

---
 lib/PublicInbox/FakeInotify.pm | 160 +++++++++++++++++----------------
 lib/PublicInbox/KQNotify.pm    |  73 ++-------------
 t/fake_inotify.t               |  22 +++--
 3 files changed, 103 insertions(+), 152 deletions(-)

diff --git a/lib/PublicInbox/FakeInotify.pm b/lib/PublicInbox/FakeInotify.pm
index 45b80f50..ae9b7dea 100644
--- a/lib/PublicInbox/FakeInotify.pm
+++ b/lib/PublicInbox/FakeInotify.pm
@@ -17,97 +17,109 @@ 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 { watch => {} }, __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 watch_open ($$$) { # used by KQNotify subclass
+	my ($self, $path, $mask) = @_;
+	my ($fh, @st, @st0, $tries);
+	do {
+		@st0 = stat($path) or return;
+		(-d _ ? opendir($fh, $path) : open($fh, '<', $path)) or return;
+		@st = stat($fh) or die "fstat($path): $!";
+	} while ("@st[0,1]" ne "@st0[0,1]" &&
+		((++$tries < 10) || return warn(<<EOM)));
+E: $path switching inodes too frequently to watch
+EOM
+	($mask & IN_DELETE && -d _) and
+		@{$self->{dirlist}->{$path}} = grep(!/\A\.\.?\z/, readdir($fh));
+
+	bless [ @st[0, 1, 10], $path, $fh ], 'PublicInbox::FakeInotify::Watch'
 }
 
 # 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);
-	}
-	bless [ $self->{watch}, $k ], 'PublicInbox::FakeInotify::Watch';
+	my ($self, $path, $mask) = @_; # mask is ignored
+	my $w = watch_open($self, $path, $mask) or return;
+	pop @$w; # no need to keep $fh open for non-kqueue
+	$self->{watch}->{$w->[3]} = $w;
 }
 
-# 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;
+	my ($self, $events, $dh, $path, $old_ctime) = @_;
+	my $old = $self->{dirlist}->{$path}; # only set if watching IN_DELETE
+	my @cur;
+	my $latest = $old_ctime;
 	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')
+		my $f = "$path/$base";
+		if (my @st = stat($f)) {
+			push @cur, $base if $old;
+			if ($st[10] > $old_ctime) {
+				$latest = $st[10] if $st[10] > $latest;
+				push(@$events, bless(\$f,
+					'PublicInbox::FakeInotify::Event'));
+			}
+		} else { # ignore ENOENT due to race
+			warn "W: stat($f): $!\n" if !$!{ENOENT};
 		}
-		if (!@st) {
-			# ignore ENOENT due to race
-			warn "unhandled stat($full) error: $!\n" if !$!{ENOENT};
-		} elsif ($newlist) {
-			$newlist->{$base} = undef;
+	}
+	# -watch only cares for new files, lei wants deletes
+	if ($old) {
+		$self->{dirlist}->{$path} = \@cur;
+		my %gone = map { $_ => undef } @$old;
+		delete @gone{@cur};
+		push(@$events, map {
+			bless \"$path/$_", 'PublicInbox::FakeInotify::GoneEvent'
+		} keys %gone);
+	}
+	$latest;
+}
+
+sub gone_event ($$$) {
+	my ($self, $ident, $path) = @_;
+	delete $self->{watch}->{$ident};
+	delete $self->{dirlist}->{$path};
+	bless(\$path, 'PublicInbox::FakeInotify::SelfGoneEvent');
+}
+
+sub ckwatch_i ($$$) { # used by KQNotify subclass
+	my ($self, $events, $ident) = @_;
+	my $w = $self->{watch}->{$ident} or return;
+	return delete($self->{watch}->{$ident}) if !@$w; # cancelled
+	# $fh is only defined for KQNotify subclass
+	my ($old_dev, $old_ino, $old_ctime, $path, $fh) = @$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, gone_event($self, $ident, $path);
+	} elsif ($fh || $new_st[10] > $old_ctime) {
+		$w->[2] = $new_st[10];
+		if (-d _) {
+			if ($fh) { # kevent requires FDs stay open
+				rewinddir($fh);
+			} elsif (!opendir($fh, $path)) {
+				push @$events, gone_event($self, $ident, $path);
+				return;
+			}
+			my $ct = on_dir_change($self, $events, $fh, $path,
+						$old_ctime);
+			$w->[2] = $ct if $ct > $w->[2];
+		} else {
+			push @$events, bless(\$path,
+					'PublicInbox::FakeInotify::Event');
 		}
 	}
-	return if !$newlist;
-	delete @$oldlist{keys %$newlist};
-	$dirlist->{$path} = $newlist;
-	push(@$events, map {
-		bless \"$path/$_", 'PublicInbox::FakeInotify::GoneEvent'
-	} keys %$oldlist);
 }
 
 # 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};
-		}
-		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";
-			}
-		}
+	for my $ident (keys %{$self->{watch}}) {
+		ckwatch_i($self, $events, $ident);
 	}
-	delete @$watch{@watch_gone};
 	@$events;
 }
 
@@ -120,15 +132,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..cec68bb6 100644
--- a/lib/PublicInbox/KQNotify.pm
+++ b/lib/PublicInbox/KQNotify.pm
@@ -5,10 +5,9 @@
 # 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);
 
 # NOTE_EXTEND detects rename(2), NOTE_WRITE detects link(2)
 sub MOVED_TO_OR_CREATE () { NOTE_EXTEND|NOTE_WRITE }
@@ -20,31 +19,14 @@ sub new {
 
 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 $w = $self->watch_open($path, $mask);
+	my $ident = fileno($w->[4]);
 	$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;
+	$self->{watch}->{$ident} = $w;
 }
 
 # emulate Linux::Inotify::fileno
@@ -61,54 +43,11 @@ 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) {
-		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});
-		}
+	for my $kev ($self->{dskq}->{kq}->kevent(0)) {
+		$self->ckwatch_i($self, $events, $kev->[KQ_IDENT]);
 	}
-	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/fake_inotify.t b/t/fake_inotify.t
index 734ddbfb..9f42c592 100644
--- a/t/fake_inotify.t
+++ b/t/fake_inotify.t
@@ -1,10 +1,10 @@
 #!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
@@ -21,32 +21,38 @@ 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: $!";
+tick $MIN_FS_TICK;
 @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;