From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id D8A691F5AE for ; Wed, 26 May 2021 18:08:57 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH] lei: require Socket::MsgHdr or Inline::C, drop oneshot Date: Wed, 26 May 2021 18:08:57 +0000 Message-Id: <20210526180857.26648-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: The cost of supporting separate code paths between oneshot and daemon isn't worth the trouble; especially if there are more users to support. The test suite time nearly doubles with oneshot, so that's hurting developer productivity. FD passing is currently required to work efficiently with remote HTTP(S) queries which return large messages, as seen in commit 708b182a57373172f5523f3dc297659d58e03b58 ("ipc: wq: handle >MAX_ARG_STRLEN && {oneshot}) { # we'll die if disconnected: + if ($lei) { # we'll die if disconnected: pipe($out_r, my $out_w) or die "pipe: $!"; $lei->send_exec_cmd([ $in_r, $out_w ], $cmd, {}); } else { diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm index c8d2f315..6ff249d0 100644 --- a/lib/PublicInbox/LEI.pm +++ b/lib/PublicInbox/LEI.pm @@ -444,19 +444,6 @@ sub x_it ($$) { dump_and_clear_log(); if (my $s = $self->{pkt_op_p} // $self->{sock}) { send($s, "x_it $code", MSG_EOR); - } elsif ($self->{oneshot}) { - # don't want to end up using $? from child processes - _drop_wq($self); - # cleanup anything that has tempfiles or open file handles - %PATH2CFG = (); - delete @$self{qw(ovv dedupe sto cfg)}; - if (my $signum = ($code & 127)) { # usually SIGPIPE (13) - $SIG{PIPE} = 'DEFAULT'; # $SIG{$signum} doesn't work - kill $signum, $$; - sleep(1) while 1; # wait for signal - } else { - $quit->($code >> 8); - } } # else ignore if client disconnected } @@ -921,17 +908,6 @@ sub start_mua { my $io = []; $io->[0] = $self->{1} if $self->{opt}->{stdin} && -t $self->{1}; send_exec_cmd($self, $io, \@cmd, {}); - } elsif ($self->{oneshot}) { - my $pid = fork // die "fork: $!"; - if ($pid > 0) { # original process - if ($self->{opt}->{stdin} && -t STDOUT) { - open STDIN, '+<&', \*STDOUT or die "dup2: $!"; - } - exec(@cmd); - warn "exec @cmd: $!\n"; - POSIX::_exit(1); - } - POSIX::setsid() > 0 or die "setsid: $!"; } if ($self->{lxs} && $self->{au_done}) { # kick wait_startq syswrite($self->{au_done}, 'q' x ($self->{lxs}->{jobs} // 0)); @@ -952,14 +928,11 @@ sub send_exec_cmd { # tell script/lei to execute a command sub poke_mua { # forces terminal MUAs to wake up and hopefully notice new mail my ($self) = @_; my $alerts = $self->{opt}->{alert} // return; + my $sock = $self->{sock}; while (my $op = shift(@$alerts)) { if ($op eq ':WINCH') { # hit the process group that started the MUA - if ($self->{sock}) { - send($self->{sock}, '-WINCH', MSG_EOR); - } elsif ($self->{oneshot}) { - kill('-WINCH', $$); - } + send($sock, '-WINCH', MSG_EOR) if $sock; } elsif ($op eq ':bell') { out($self, "\a"); } elsif ($op =~ /(?{sock}) { - send($s, exec_buf($cmd, {}), MSG_EOR); - } elsif ($self->{oneshot}) { - $self->{"pid.$self.$$"}->{spawn($cmd)} = $cmd; - } + send($sock, exec_buf($cmd, {}), MSG_EOR) if $sock; } else { err($self, "W: unsupported --alert=$op"); # non-fatal } @@ -1009,9 +978,6 @@ sub start_pager { if ($self->{sock}) { # lei(1) process runs it delete @$new_env{keys %$env}; # only set iff unset send_exec_cmd($self, [ @$rdr{0..2} ], [$pager], $new_env); - } elsif ($self->{oneshot}) { - my $cmd = [$pager]; - $self->{"pid.$self.$$"}->{spawn($cmd, $new_env, $rdr)} = $cmd; } else { die 'BUG: start_pager w/o socket'; } @@ -1253,29 +1219,13 @@ sub lazy_start { sub busy { 1 } # prevent daemon-shutdown if client is connected -# for users w/o Socket::Msghdr installed or Inline::C enabled -sub oneshot { - my ($main_pkg) = @_; - my $exit = $main_pkg->can('exit'); # caller may override exit() - local $quit = $exit if $exit; - local %PATH2CFG; - umask(077) // die("umask(077): $!"); - my $self = bless { oneshot => 1, env => \%ENV }, __PACKAGE__; - for (0..2) { open($self->{$_}, '+<&=', $_) or die "open fd=$_: $!" } - dispatch($self, @ARGV); - x_it($self, $self->{child_error}) if $self->{child_error}; -} - # ensures stdout hits the FS before sock disconnects so a client # can immediately reread it sub DESTROY { my ($self) = @_; $self->{1}->autoflush(1) if $self->{1}; stop_pager($self); - my $err = $?; - my $oneshot_pids = delete $self->{"pid.$self.$$"} or return; - waitpid($_, 0) for keys %$oneshot_pids; - $? = $err if $err; # preserve ->fail or ->x_it code + # preserve $? for ->fail or ->x_it code } sub wq_done_wait { # dwaitpid callback diff --git a/lib/PublicInbox/LeiEditSearch.pm b/lib/PublicInbox/LeiEditSearch.pm index 30ac65bd..13713d24 100644 --- a/lib/PublicInbox/LeiEditSearch.pm +++ b/lib/PublicInbox/LeiEditSearch.pm @@ -14,19 +14,12 @@ sub lei_edit_search { my @cmd = (qw(git config --edit -f), $lss->{'-f'}); $lei->qerr("# spawning @cmd"); $lss->edit_begin($lei); - if ($lei->{oneshot}) { - require PublicInbox::Spawn; - waitpid(PublicInbox::Spawn::spawn(\@cmd), 0); - # non-fatal, editor could fail after successful write - $lei->child_error($?) if $?; - $lss->edit_done($lei); - } else { # run in script/lei foreground - require PublicInbox::PktOp; - my ($op_c, $op_p) = PublicInbox::PktOp->pair; - # $op_p will EOF when $EDITOR is done - $op_c->{ops} = { '' => [$lss->can('edit_done'), $lss, $lei] }; - $lei->send_exec_cmd([ @$lei{qw(0 1 2)}, $op_p ], \@cmd, {}); - } + # run in script/lei foreground + require PublicInbox::PktOp; + my ($op_c, $op_p) = PublicInbox::PktOp->pair; + # $op_p will EOF when $EDITOR is done + $op_c->{ops} = { '' => [$lss->can('edit_done'), $lss, $lei] }; + $lei->send_exec_cmd([ @$lei{qw(0 1 2)}, $op_p ], \@cmd, {}); } *_complete_edit_search = \&PublicInbox::LeiUp::_complete_up; diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm index a7a0ebef..af5edbc2 100644 --- a/lib/PublicInbox/LeiStore.pm +++ b/lib/PublicInbox/LeiStore.pm @@ -431,20 +431,15 @@ sub write_prepare { my $d = $lei->store_path; $self->ipc_lock_init("$d/ipc.lock"); substr($d, -length('/lei/store'), 10, ''); - my $err_pipe; - unless ($lei->{oneshot}) { - pipe(my ($r, $w)) or die "pipe: $!"; - $err_pipe = [ $r, $w ]; - } + pipe(my ($r, $w)) or die "pipe: $!"; + my $err_pipe = [ $r, $w ]; # Mail we import into lei are private, so headers filtered out # by -mda for public mail are not appropriate local @PublicInbox::MDA::BAD_HEADERS = (); $self->ipc_worker_spawn("lei/store $d", $lei->oldset, { lei => $lei, err_pipe => $err_pipe }); - if ($err_pipe) { - require PublicInbox::LeiStoreErr; - PublicInbox::LeiStoreErr->new($err_pipe->[0], $lei); - } + require PublicInbox::LeiStoreErr; + PublicInbox::LeiStoreErr->new($err_pipe->[0], $lei); } $lei->{sto} = $self; } diff --git a/lib/PublicInbox/LeiSucks.pm b/lib/PublicInbox/LeiSucks.pm index 2ce64d62..a71158f3 100644 --- a/lib/PublicInbox/LeiSucks.pm +++ b/lib/PublicInbox/LeiSucks.pm @@ -23,8 +23,7 @@ sub lei_sucks { } eval { require PublicInbox }; my $pi_ver = eval('$PublicInbox::VERSION') // '(???)'; - my $daemon = $lei->{oneshot} ? 'oneshot' : 'daemon'; - my @out = ("lei $pi_ver mode=$daemon\n", + my @out = ("lei $pi_ver\n", "perl $Config{version} / $os $rel / $mac ". "ptrsize=$Config{ptrsize}\n"); chomp(my $gv = `git --version` || "git missing"); diff --git a/lib/PublicInbox/LeiUp.pm b/lib/PublicInbox/LeiUp.pm index 4399c4fb..9069232b 100644 --- a/lib/PublicInbox/LeiUp.pm +++ b/lib/PublicInbox/LeiUp.pm @@ -76,22 +76,18 @@ sub lei_up { my @all = PublicInbox::LeiSavedSearch::list($lei); my @local = grep(!m!\Aimaps?://!i, @all); $lei->_lei_store->write_prepare($lei); # share early - if ($lei->{oneshot}) { # synchronous - up1_redispatch($lei, $_) for @local; - } else { - # daemon mode, re-dispatch into our event loop w/o - # creating an extra fork-level - require PublicInbox::DS; - require PublicInbox::PktOp; - my ($op_c, $op_p) = PublicInbox::PktOp->pair; - for my $o (@local) { - PublicInbox::DS::requeue(sub { - up1_redispatch($lei, $o, $op_p); - }); - } - $lei->event_step_init; - $op_c->{ops} = { '' => [$lei->can('dclose'), $lei] }; + # daemon mode, re-dispatch into our event loop w/o + # creating an extra fork-level + require PublicInbox::DS; + require PublicInbox::PktOp; + my ($op_c, $op_p) = PublicInbox::PktOp->pair; + for my $o (@local) { + PublicInbox::DS::requeue(sub { + up1_redispatch($lei, $o, $op_p); + }); } + $lei->event_step_init; + $op_c->{ops} = { '' => [$lei->can('dclose'), $lei] }; } else { up1($lei, $out); } diff --git a/lib/PublicInbox/TestCommon.pm b/lib/PublicInbox/TestCommon.pm index 460c9da0..83dcf650 100644 --- a/lib/PublicInbox/TestCommon.pm +++ b/lib/PublicInbox/TestCommon.pm @@ -544,7 +544,8 @@ EOM ($tmpdir, $for_destroy) = tmpdir unless $tmpdir; state $persist_xrd = $ENV{TEST_LEI_DAEMON_PERSIST_DIR}; SKIP: { - skip 'TEST_LEI_ONESHOT set', 1 if $ENV{TEST_LEI_ONESHOT}; + $ENV{TEST_LEI_ONESHOT} and + xbail 'TEST_LEI_ONESHOT no longer supported'; my $home = "$tmpdir/lei-daemon"; mkdir($home, 0700) or BAIL_OUT "mkdir: $!"; local $ENV{HOME} = $home; @@ -568,22 +569,12 @@ EOM lei_ok(qw(daemon-kill), \"daemon-kill after $t"); } }; # SKIP for lei_daemon - unless ($test_opt->{daemon_only}) { - $ENV{TEST_LEI_DAEMON_ONLY} and - skip 'TEST_LEI_DAEMON_ONLY set', 1; - require_ok 'PublicInbox::LEI'; - my $home = "$tmpdir/lei-oneshot"; - mkdir($home, 0700) or BAIL_OUT "mkdir: $!"; - local $ENV{HOME} = $home; - local $ENV{XDG_RUNTIME_DIR} = '/dev/null'; - $cb->(); - } if ($daemon_pid) { for (0..10) { kill(0, $daemon_pid) or last; tick; } - ok(!kill(0, $daemon_pid), "$t daemon stopped after oneshot"); + ok(!kill(0, $daemon_pid), "$t daemon stopped"); my $f = "$daemon_xrd/lei/errors.log"; open my $fh, '<', $f or BAIL_OUT "$f: $!"; my @l = <$fh>; diff --git a/script/lei b/script/lei index bec6b001..4d752fd8 100755 --- a/script/lei +++ b/script/lei @@ -9,10 +9,18 @@ my $narg = 5; my $sock; my $recv_cmd = PublicInbox::CmdIPC4->can('recv_cmd4'); my $send_cmd = PublicInbox::CmdIPC4->can('send_cmd4') // do { + my $inline_dir = $ENV{PERL_INLINE_DIRECTORY} //= ( + $ENV{XDG_CACHE_HOME} // + ( ($ENV{HOME} // '/nonexistent').'/.cache' ) + ).'/public-inbox/inline-c'; + if (!-d $inline_dir) { + require File::Path; + File::Path::make_path($inline_dir); + } require PublicInbox::Spawn; # takes ~50ms even if built *sigh* $recv_cmd = PublicInbox::Spawn->can('recv_cmd4'); PublicInbox::Spawn->can('send_cmd4'); -}; +} // die 'please install Inline::C or Socket::MsgHdr'; my %pids; my $sigchld = sub { @@ -66,80 +74,69 @@ my $exec_cmd = sub { } }; -if ($send_cmd && eval { - my $path = do { - my $runtime_dir = ($ENV{XDG_RUNTIME_DIR} // '') . '/lei'; - die \0 if $runtime_dir eq '/dev/null/lei'; # oneshot forced - if ($runtime_dir eq '/lei') { - require File::Spec; - $runtime_dir = File::Spec->tmpdir."/lei-$<"; - } - unless (-d $runtime_dir) { - require File::Path; - File::Path::mkpath($runtime_dir, 0, 0700); - } - "$runtime_dir/$narg.seq.sock"; - }; - my $addr = pack_sockaddr_un($path); - socket($sock, AF_UNIX, SOCK_SEQPACKET, 0) or die "socket: $!"; - unless (connect($sock, $addr)) { # start the daemon if not started - local $ENV{PERL5LIB} = join(':', @INC); - open(my $daemon, '-|', $^X, qw[-MPublicInbox::LEI - -E PublicInbox::LEI::lazy_start(@ARGV)], - $path, $! + 0, $narg) or die "popen: $!"; - while (<$daemon>) { warn $_ } # EOF when STDERR is redirected - close($daemon) or warn <<""; +my $runtime_dir = ($ENV{XDG_RUNTIME_DIR} // '') . '/lei'; +if ($runtime_dir eq '/lei') { + require File::Spec; + $runtime_dir = File::Spec->tmpdir."/lei-$<"; +} +unless (-d $runtime_dir) { + require File::Path; + File::Path::make_path($runtime_dir, { mode => 0700 }); +} +my $path = "$runtime_dir/$narg.seq.sock"; +my $addr = pack_sockaddr_un($path); +socket($sock, AF_UNIX, SOCK_SEQPACKET, 0) or die "socket: $!"; +unless (connect($sock, $addr)) { # start the daemon if not started + local $ENV{PERL5LIB} = join(':', @INC); + open(my $daemon, '-|', $^X, qw[-MPublicInbox::LEI + -E PublicInbox::LEI::lazy_start(@ARGV)], + $path, $! + 0, $narg) or die "popen: $!"; + while (<$daemon>) { warn $_ } # EOF when STDERR is redirected + close($daemon) or warn <<""; lei-daemon could not start, exited with \$?=$? - # try connecting again anyways, unlink+bind may be racy - connect($sock, $addr) or die <<""; + # try connecting again anyways, unlink+bind may be racy + connect($sock, $addr) or die <<""; connect($path): $! (after attempted daemon start) Falling back to (slow) one-shot mode +} +# (Socket::MsgHdr|Inline::C), $sock are all available: +open my $dh, '<', '.' or die "open(.) $!"; +my $buf = join("\0", scalar(@ARGV), @ARGV); +while (my ($k, $v) = each %ENV) { $buf .= "\0$k=$v" } +$buf .= "\0\0"; +my $n = $send_cmd->($sock, [0, 1, 2, fileno($dh)], $buf, MSG_EOR); +if (!$n) { + die "sendmsg: $! (check RLIMIT_NOFILE)\n" if $!{ETOOMANYREFS}; + die "sendmsg: $!\n"; +} +my $x_it_code = 0; +while (1) { + my (@fds) = $recv_cmd->($sock, my $buf, 4096 * 33); + if (scalar(@fds) == 1 && !defined($fds[0])) { + next if $!{EINTR}; + last if $!{ECONNRESET}; + die "recvmsg: $!"; } - # (Socket::MsgHdr|Inline::C), $sock are all available: - open my $dh, '<', '.' or die "open(.) $!"; - my $buf = join("\0", scalar(@ARGV), @ARGV); - while (my ($k, $v) = each %ENV) { $buf .= "\0$k=$v" } - $buf .= "\0\0"; - my $n = $send_cmd->($sock, [0, 1, 2, fileno($dh)], $buf, MSG_EOR); - if (!$n) { - die "sendmsg: $! (check RLIMIT_NOFILE)\n" if $!{ETOOMANYREFS}; - die "sendmsg: $!\n"; - } - 1; -}) { # connected and request sent to lei-daemon, wait for responses or EOF - my $x_it_code = 0; - while (1) { - my (@fds) = $recv_cmd->($sock, my $buf, 4096 * 33); - if (scalar(@fds) == 1 && !defined($fds[0])) { - next if $!{EINTR}; - last if $!{ECONNRESET}; - die "recvmsg: $!"; - } - last if $buf eq ''; - if ($buf =~ /\Aexec (.+)\z/) { - $exec_cmd->(\@fds, split(/\0/, $1)); - } elsif ($buf eq '-WINCH') { - kill($buf, @parent); # for MUA - } elsif ($buf =~ /\Ax_it ([0-9]+)\z/) { - $x_it_code ||= $1 + 0; - last; - } elsif ($buf =~ /\Achild_error ([0-9]+)\z/) { - $x_it_code ||= $1 + 0; - } else { - $sigchld->(); - die $buf; - } - } - $sigchld->(); - if (my $sig = ($x_it_code & 127)) { - kill $sig, $$; - sleep(1) while 1; + last if $buf eq ''; + if ($buf =~ /\Aexec (.+)\z/) { + $exec_cmd->(\@fds, split(/\0/, $1)); + } elsif ($buf eq '-WINCH') { + kill($buf, @parent); # for MUA + } elsif ($buf =~ /\Ax_it ([0-9]+)\z/) { + $x_it_code ||= $1 + 0; + last; + } elsif ($buf =~ /\Achild_error ([0-9]+)\z/) { + $x_it_code ||= $1 + 0; + } else { + $sigchld->(); + die $buf; } - exit($x_it_code >> 8); -} else { # for systems lacking Socket::MsgHdr or Inline::C - warn $@ if $@ && !ref($@); - require PublicInbox::LEI; - PublicInbox::LEI::oneshot(__PACKAGE__); } +$sigchld->(); +if (my $sig = ($x_it_code & 127)) { + kill $sig, $$; + sleep(1) while 1; +} +exit($x_it_code >> 8); diff --git a/t/lei-daemon.t b/t/lei-daemon.t index 84e2791d..a7c4b799 100644 --- a/t/lei-daemon.t +++ b/t/lei-daemon.t @@ -73,15 +73,6 @@ test_lei({ daemon_only => 1 }, sub { lei_ok('daemon-pid'); chomp $lei_out; is($lei_out, $new_pid, 'PID unchanged after -0/-CHLD'); - - SKIP: { # socket inaccessible - skip "cannot test connect EPERM as root", 3 if $> == 0; - chmod 0000, $sock or BAIL_OUT "chmod 0000: $!"; - lei_ok('help', \'connect fail, one-shot fallback works'); - like($lei_err, qr/\bconnect\(/, 'connect error noted'); - like($lei_out, qr/^usage: /, 'help output works'); - chmod 0700, $sock or BAIL_OUT "chmod 0700: $!"; - } unlink $sock or BAIL_OUT "unlink($sock) $!"; for (0..100) { kill('CHLD', $new_pid) or last;