lei_mirror: eliminate circular references

...by using local-ized globals.  While non-globals could work,
eliminating the {todo} and {fgrp_todo} refs in all sub-refs
is more error-prone and the `local' construct is convenient.

This allows us to get rid of the `delete $fgrp->{-fini}' call
in pack_refs and eliminates the indiscriminate reaping of all
processes before calling fgrp_fetch_all.  This means we can
fully depend on DESTROY to provide predictable dependency
handling while supporting parallelization.

Global $TODO and $FGRP_TODO now become SCALAR refs on
consumption so they can act as assertions to detect future bugs.

diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 47db9ccd8c3ef8e002d3e034a4b81679d278c766..b30cc519b99de1dae1a294ff056ca946df3f3fad 100644 (file)
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -21,6 +21,8 @@ use PublicInbox::OnDestroy;
 use Digest::SHA qw(sha256_hex sha1_hex);
 
 our $LIVE; # pid => callback
+our $FGRP_TODO; # objstore -> [ fgrp mirror objects ]
+our $TODO; # reference => [ non-fgrp mirror objects ]
 
 sub keep_going ($) {
        $LIVE && (!$_[0]->{lei}->{child_error} ||
@@ -324,7 +326,6 @@ sub fgrp_update {
 sub pack_dst { # packs lightweight satellite repos
        my ($fgrp) = @_;
        pack_refs($fgrp, $fgrp->{cur_dst});
-       delete($fgrp->{-fini}) // die 'BUG: no {-fini}'; # call v1_done
 }
 
 sub pack_refs {
@@ -362,7 +363,8 @@ sub fgrpv_done {
 
 sub fgrp_fetch_all {
        my ($self) = @_;
-       my $todo = delete $self->{fgrp_todo} or return;
+       my $todo = $FGRP_TODO;
+       $FGRP_TODO = \'BUG on further use';
        keys(%$todo) or return;
 
        # Rely on the fgrptmp remote groups in the config file rather
@@ -510,7 +512,7 @@ sub resume_fetch {
 }
 
 sub fgrp_enqueue {
-       my ($fgrp) = @_;
+       my ($fgrp, $end) = @_; # $end calls fgrp_fetch_all
        return if !keep_going($fgrp);
        my $opt = { 2 => $fgrp->{lei}->{2} };
        # --no-tags is required to avoid conflicts
@@ -523,12 +525,11 @@ sub fgrp_enqueue {
                $fgrp->{dry_run} ? $fgrp->{lei}->qerr("# @cmd @kv") :
                                run_die([@cmd, @kv], undef, $opt);
        }
-       $fgrp->{fgrp_todo} // die 'BUG: no fgrp_todo';
-       push @{$fgrp->{fgrp_todo}->{$fgrp->{-osdir}}}, $fgrp;
+       push @{$FGRP_TODO->{$fgrp->{-osdir}}}, $fgrp;
 }
 
 sub clone_v1 {
-       my ($self, $nohang) = @_;
+       my ($self, $end) = @_;
        my $lei = $self->{lei};
        my $curl = $self->{curl} //= PublicInbox::LeiCurl->new($lei) or return;
        my $uri = URI->new($self->{cur_src} // $self->{src});
@@ -540,7 +541,8 @@ sub clone_v1 {
        my $resume = -d $dst;
        if (my $fgrp = forkgroup_prep($self, $uri)) {
                $fgrp->{-fini} = $fini;
-               $resume ? cmp_fp_do($fgrp, \&fgrp_enqueue) : fgrp_enqueue($fgrp)
+               $resume ? cmp_fp_do($fgrp, \&fgrp_enqueue, $end)
+                       : fgrp_enqueue($fgrp, $end);
        } elsif ($resume) {
                cmp_fp_do($self, \&resume_fetch, $uri, $fini);
        } else { # normal clone
@@ -562,10 +564,10 @@ sub clone_v1 {
 
        my $d = $self->{-ent} ? $self->{-ent}->{description} : undef;
        $self->{'txt.description'} = $d if defined $d;
-       (!defined($d) && !$nohang) and
+       (!defined($d) && !$end) and
                _get_txt_start($self, 'description', $fini);
 
-       $nohang or do_reap($self, 1); # for non-manifest clone
+       $end or do_reap($self, 1); # for non-manifest clone
 }
 
 sub parse_epochs ($$) {
@@ -785,7 +787,6 @@ sub clone_v2_prep ($$;$) {
        my $dst = $self->{cur_dst} // $self->{dst};
        my $want = parse_epochs($lei->{opt}->{epoch}, $v2_epochs);
        my $task = $m ? bless { %$self }, __PACKAGE__ : $self;
-       delete $task->{todo}; # $self->{todo} still exists
        my (@skip, $desc);
        my $fini = PublicInbox::OnDestroy->new($$, \&v2_done, $task);
        for my $nr (sort { $a <=> $b } keys %$v2_epochs) {
@@ -813,7 +814,7 @@ failed to extract epoch number from $src
                        $etask->{cur_dst} = $edst;
                        $etask->{-is_epoch} = $fini;
                        my $ref = $ent->{reference} // '';
-                       push @{$self->{todo}->{$ref}}, $etask;
+                       push @{$TODO->{$ref}}, $etask;
                        $self->{any_want}->{$key} = 1;
                } else { # create a placeholder so users only need to chmod +w
                        init_placeholder($src, $edst, $ent);
@@ -917,7 +918,9 @@ sub multi_inbox ($$$) {
 
 sub clone_all {
        my ($self, $m) = @_;
-       my $todo = delete $self->{todo};
+       my $todo = $TODO;
+       $TODO = \'BUG on further use';
+       my $end = PublicInbox::OnDestroy->new($$, \&fgrp_fetch_all, $self);
        {
                my $nodep = delete $todo->{''};
 
@@ -929,7 +932,7 @@ sub clone_all {
 
                # handle no-dependency repos, first
                for (@$nodep) {
-                       clone_v1($_, 1);
+                       clone_v1($_, $end);
                        return if !keep_going($self);
                }
        }
@@ -947,14 +950,16 @@ EOM
                        }
                        my $y = delete $todo->{$x} // next; # already done
                        for (@$y) {
-                               clone_v1($_, 1);
+                               clone_v1($_, $end);
                                return if !keep_going($self);
                        }
                        last; # restart %$todo iteration
                }
        }
-       do_reap($self, 1); # finish all fingerprint checks
-       fgrp_fetch_all($self);
+
+       # $end->DESTROY will call fgrp_fetch_all once all references
+       # in $LIVE are gone, and do_reap will eventually drain $LIVE
+       $end = undef;
        do_reap($self, 1);
 }
 
@@ -1007,8 +1012,6 @@ sub try_manifest {
        my ($path_pfx, $n, $multi) = multi_inbox($self, \$path, $m);
        return $lei->child_error(1, $multi) if !ref($multi);
        my $v2 = delete $multi->{v2};
-       local $self->{todo} = {};
-       local $self->{fgrp_todo} = {}; # { objstore_dir => [fgrp, ...] }
        if ($v2) {
                for my $name (sort keys %$v2) {
                        my $epochs = delete $v2->{$name};
@@ -1054,7 +1057,7 @@ E: `$task->{cur_dst}' must not contain newline
 EOM
                        $task->{cur_src} .= '/';
                        my $dep = $task->{-ent}->{reference} // '';
-                       push @{$self->{todo}->{$dep}}, $task; # for clone_all
+                       push @{$TODO->{$dep}}, $task; # for clone_all
                        $self->{any_want}->{$name} = 1;
                }
        }
@@ -1112,6 +1115,8 @@ sub do_mirror { # via wq_io_do or public-inbox-clone
                        $self->{"-$k"} = $v;
                }
                local $LIVE = {};
+               local $TODO = {};
+               local $FGRP_TODO = {};
                my $iv = $lei->{opt}->{'inbox-version'} //
                        return start_clone_url($self);
                return clone_v1($self) if $iv == 1;