From a972a46180718b2d8d7d7d150d39d58cb09566bc Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 8 Feb 2025 03:26:35 +0000 Subject: [PATCH] daemon: check connections before solving codeblobs The /$MSGID/s/ codeblob reconstruction endpoint is extremely expensive and one of the few which do a large amount of upfront processing before being able to write to an HTTP client and detect if they're still alive (unlike other expensive endpoints). For TCP connections, use the TCP_INFO via getsockopt(2) if supported or attempt to use poll(2) + the FIONREAD ioctl(2) to check the connection before possibly invoking git-(apply|ls-files|update-index) dozens or even hundreds of times to reconstruct a blob. --- MANIFEST | 2 ++ devel/sysdefs-list | 16 +++++++++- lib/PublicInbox/Daemon.pm | 29 ++++++++++++++++++ lib/PublicInbox/HTTP.pm | 7 +++-- lib/PublicInbox/HTTPD.pm | 2 ++ lib/PublicInbox/Syscall.pm | 10 ++++++- lib/PublicInbox/TestCommon.pm | 3 +- lib/PublicInbox/ViewVCS.pm | 13 +++++--- t/daemon.t | 56 +++++++++++++++++++++++++++++++++++ t/psgi_log.psgi | 16 ++++++++++ t/solver_git.t | 49 ++++++++++++++++++++++++++++-- 11 files changed, 192 insertions(+), 11 deletions(-) create mode 100644 t/daemon.t create mode 100644 t/psgi_log.psgi diff --git a/MANIFEST b/MANIFEST index 30adab809..1fc2a152b 100644 --- a/MANIFEST +++ b/MANIFEST @@ -448,6 +448,7 @@ t/config.t t/config_limiter.t t/content_hash.t t/convert-compact.t +t/daemon.t t/data-gen/.gitignore t/data/0001.patch t/data/attached-mbox-with-utf8.eml @@ -587,6 +588,7 @@ t/precheck.t t/psgi_attach.eml t/psgi_attach.t t/psgi_bad_mids.t +t/psgi_log.psgi t/psgi_mount.t t/psgi_multipart_not.t t/psgi_scan_all.t diff --git a/devel/sysdefs-list b/devel/sysdefs-list index 1345e0ba2..3a2e60d20 100755 --- a/devel/sysdefs-list +++ b/devel/sysdefs-list @@ -24,6 +24,14 @@ my $x = "$tmp/sysdefs"; open my $fh, '>', $f or die "open $f $!"; print $fh $str or die "print $f $!"; close $fh or die "close $f $!"; +for (qw(sys/ioctl sys/filio)) { + my $cfg_name = $_; + my $cpp_name = uc $_; + $cfg_name =~ tr!/!!d; + $cpp_name =~ tr!/!_!; + ($Config{"i_$cfg_name"} // '') eq 'define' and + push @cflags, "-DHAVE_${cpp_name}_H"; +} system($cc, '-o', $x, $f, @cflags) == 0 or die "$cc failed \$?=$?"; print STDERR '# %Config', (map { " $_=$Config{$_}" } qw(ptrsize sizesize lseeksize)), "\n"; @@ -37,7 +45,12 @@ __DATA__ #include #include #include -#include +#ifdef HAVE_SYS_IOCTL_H +# include +#endif +#ifdef HAVE_SYS_FILIO_H +# include +#endif #ifdef __linux__ # include # include @@ -144,6 +157,7 @@ int main(void) #endif /* Linux, any other OSes with stable syscalls? */ D(SIGWINCH); + MAYBE X(FIONREAD); MAYBE D(SO_ACCEPTFILTER); MAYBE D(_SC_NPROCESSORS_ONLN); MAYBE D(_SC_AVPHYS_PAGES); diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm index 57f01d2cb..84147b684 100644 --- a/lib/PublicInbox/Daemon.pm +++ b/lib/PublicInbox/Daemon.pm @@ -11,6 +11,7 @@ use Getopt::Long qw(:config gnu_getopt no_ignore_case auto_abbrev); use IO::Handle; # ->autoflush use IO::Socket; use File::Spec; +use IO::Poll qw(POLLERR POLLIN POLLHUP); use POSIX qw(WNOHANG :signal_h F_SETFD); use Socket qw(IPPROTO_TCP SOL_SOCKET); STDOUT->autoflush(1); @@ -754,4 +755,32 @@ sub write_pid ($) { do_chown($path); } +sub stream_hup ($) { + my $ev = POLLIN; + my $n = IO::Poll::_poll(0, fileno($_[0]) // return, $ev) or return; + return 1 if $ev & (POLLHUP|POLLERR); + + # n.b. POLLHUP isn't reliably detected, so check FIONREAD on POLLIN + if (defined(PublicInbox::Syscall::FIONREAD) && ($ev & POLLIN)) { + ioctl($_[0], PublicInbox::Syscall::FIONREAD, $n = "") // + return; + return (unpack('i', $n) == 0); + } + undef; +} + +if (PublicInbox::Syscall->can('TCP_ESTABLISHED')) { + eval <<'EOM'; +sub tcp_hup ($) { + my $buf = getsockopt($_[0], Socket::IPPROTO_TCP, Socket::TCP_INFO) + or return; + unpack('C', $buf) != PublicInbox::Syscall::TCP_ESTABLISHED +} +EOM + warn "E: $@" if $@; +} + +no warnings 'once'; +*tcp_hup = \&stream_hup if !__PACKAGE__->can('tcp_hup'); + 1; diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm index 80ebad164..105156e24 100644 --- a/lib/PublicInbox/HTTP.pm +++ b/lib/PublicInbox/HTTP.pm @@ -165,11 +165,14 @@ sub app_dispatch { } } +# we use the non-standard 499 code for client disconnects (matching nginx) +sub status_msg ($) { status_message($_[0]) // 'error' } + sub response_header_write ($$$) { my ($self, $env, $res) = @_; my $proto = $env->{SERVER_PROTOCOL} or return; # HTTP/0.9 :P my $status = $res->[0]; - my $h = "$proto $status " . status_message($status) . "\r\n"; + my $h = "$proto $status " . status_msg($status) . "\r\n"; my ($len, $chunked); my $headers = $res->[1]; @@ -428,7 +431,7 @@ sub read_input_chunked { # unlikely... sub quit { my ($self, $status) = @_; - my $h = "HTTP/1.1 $status " . status_message($status) . "\r\n\r\n"; + my $h = "HTTP/1.1 $status " . status_msg($status) . "\r\n\r\n"; $self->write(\$h); $self->close; undef; # input_prepare expects this diff --git a/lib/PublicInbox/HTTPD.pm b/lib/PublicInbox/HTTPD.pm index 6a6347d8b..38b2e4f9a 100644 --- a/lib/PublicInbox/HTTPD.pm +++ b/lib/PublicInbox/HTTPD.pm @@ -45,6 +45,8 @@ sub env_for ($$$) { 'pi-httpd.async' => 1, 'pi-httpd.app' => $self->{app}, 'pi-httpd.warn_cb' => $self->{warn_cb}, + 'pi-httpd.ckhup' => $port ? \&PublicInbox::Daemon::tcp_hup : + \&PublicInbox::Daemon::stream_hup, } } diff --git a/lib/PublicInbox/Syscall.pm b/lib/PublicInbox/Syscall.pm index d005fecb6..15ff2005f 100644 --- a/lib/PublicInbox/Syscall.pm +++ b/lib/PublicInbox/Syscall.pm @@ -305,6 +305,8 @@ BEGIN { if ($^O eq 'linux') { %CONST = ( MSG_MORE => 0x8000, + FIONREAD => 0x541b, + TCP_ESTABLISHED => 1, TMPL_cmsg_len => TMPL_size_t, # cmsg_len, cmsg_level, cmsg_type SIZEOF_cmsghdr => SIZEOF_int * 2 + SIZEOF_size_t, @@ -319,6 +321,7 @@ BEGIN { } elsif ($^O =~ /\A(?:freebsd|openbsd|netbsd|dragonfly)\z/) { %CONST = ( TMPL_cmsg_len => 'L', # socklen_t + FIONREAD => 0x4004667f, SIZEOF_cmsghdr => SIZEOF_int * 3, CMSG_DATA_off => SIZEOF_ptr == 8 ? '@16' : '', TMPL_msghdr => 'PL' . # msg_name, msg_namelen @@ -328,7 +331,11 @@ BEGIN { TMPL_size_t. # msg_controllen 'i', # msg_flags - ) + ); + # *BSD uses `TCPS_ESTABLISHED', not `TCP_ESTABLISHED' + # dragonfly uses TCPS_ESTABLISHED==5, but it lacks TCP_INFO, + # so leave it unset on dfly + $CONST{TCP_ESTABLISHED} = 4 if $^O ne 'dragonfly'; } $CONST{CMSG_ALIGN_size} = SIZEOF_size_t; $CONST{SIZEOF_cmsghdr} //= 0; @@ -336,6 +343,7 @@ BEGIN { $CONST{CMSG_DATA_off} //= undef; $CONST{TMPL_msghdr} //= undef; $CONST{MSG_MORE} //= 0; + $CONST{FIONREAD} //= undef; } # SFD_CLOEXEC is arch-dependent, so IN_CLOEXEC may be, too diff --git a/lib/PublicInbox/TestCommon.pm b/lib/PublicInbox/TestCommon.pm index a887fef8f..cebd09367 100644 --- a/lib/PublicInbox/TestCommon.pm +++ b/lib/PublicInbox/TestCommon.pm @@ -21,7 +21,7 @@ $ENV{XDG_CACHE_HOME} //= "$ENV{HOME}/.cache"; # reuse C++ xap_helper builds $ENV{GIT_TEST_FSYNC} = 0; # hopefully reduce wear $_ = File::Spec->rel2abs($_) for (grep(!m!^/!, @INC)); -our $CURRENT_DAEMON; +our ($CURRENT_DAEMON, $CURRENT_LISTENER); BEGIN { @EXPORT = qw(tmpdir tcp_server tcp_connect require_git require_mods run_script start_script key2sub xsys xsys_e xqx eml_load tick @@ -989,6 +989,7 @@ sub test_httpd ($$;$$) { my $ua = LWP::UserAgent->new; $ua->max_redirect(0); local $CURRENT_DAEMON = $td; + local $CURRENT_LISTENER = $sock; Plack::Test::ExternalServer::test_psgi(client => $client, ua => $ua); $cb->() if $cb; diff --git a/lib/PublicInbox/ViewVCS.pm b/lib/PublicInbox/ViewVCS.pm index 96b60ee07..a6e54b300 100644 --- a/lib/PublicInbox/ViewVCS.pm +++ b/lib/PublicInbox/ViewVCS.pm @@ -642,12 +642,17 @@ sub show_blob { # git->cat_async callback sub start_solver ($) { my ($ctx) = @_; + $ctx->{-next_solver} = on_destroy \&next_solver; + ++$solver_nr; + if (my $ck = $ctx->{env}->{'pi-httpd.ckhup'}) { + $ck->($ctx->{env}->{'psgix.io'}->{sock}) and + return html_page $ctx, 499, 'client disconnected'; + } + while (my ($from, $to) = each %QP_MAP) { my $v = $ctx->{qp}->{$from} // next; $ctx->{hints}->{$to} = $v if $v ne ''; } - $ctx->{-next_solver} = on_destroy \&next_solver; - ++$solver_nr; # ->newdir and open may croak $ctx->{-tmp} = File::Temp->newdir("solver.$ctx->{oid_b}-XXXX", TMPDIR => 1); @@ -665,9 +670,9 @@ sub start_solver ($) { sub next_solver { --$solver_nr; my $ctx = shift(@solver_q) // return; - # XXX FIXME: client may've disconnected if it waited a long while eval { start_solver($ctx) }; - warn "W: start_solver: $@" if $@; + return unless $@; + warn "W: start_solver: $@"; html_page($ctx, 500) if $ctx->{-wcb}; } diff --git a/t/daemon.t b/t/daemon.t new file mode 100644 index 000000000..51aa70b4e --- /dev/null +++ b/t/daemon.t @@ -0,0 +1,56 @@ +# Copyright (C) all contributors +# License: AGPL-3.0+ +# unit tests for common (public-facing) daemon code +use v5.12; +use autodie; +use PublicInbox::TestCommon; +use Socket qw(SOCK_STREAM); +require_ok 'PublicInbox::Daemon'; +use PublicInbox::IO qw(poll_in); + +# $fn is stream_hup or tcp_hup +my $ck_hup = sub { + my ($s, $connect, $fn, $type) = @_; + my $c = $connect->(); + poll_in $s; # needed on *BSD + my $addr = accept(my $acc, $s); + close $c; + my $ck = PublicInbox::Daemon->can($fn); + poll_in($acc) if $^O ne 'linux'; # netbsd needs this, at least... + ok $ck->($acc), "$fn detected close ($type)"; + $c = $connect->(); + syswrite $c, 'hi'; + poll_in $s; # needed on *BSD + $addr = accept($acc, $s); + ok !$ck->($acc), "$fn false when still established ($type)"; +}; + +{ + my $tmpdir = tmpdir; + my $l = "$tmpdir/named.sock"; + my $s = IO::Socket::UNIX->new(Listen => 5, Local => $l, + Type => SOCK_STREAM) or + xbail "bind+listen($l): $!"; + my $connect = sub { + IO::Socket::UNIX->new(Peer => $l, Type => SOCK_STREAM) or + xbail "connect($l): $!"; + }; + $ck_hup->($s, $connect, 'stream_hup', 'UNIX'); +} + +{ + my $s = tcp_server; + my $tcp_conn = sub { tcp_connect($s) }; + $ck_hup->($s, $tcp_conn, 'stream_hup', 'TCP'); + $ck_hup->($s, $tcp_conn, 'tcp_hup', 'TCP'); +} + +SKIP: { + $^O =~ /\A(?:linux|freebsd|netbsd|openbsd)\z/ or + skip "no TCP_INFO support $^O", 1; + isnt \&PublicInbox::Daemon::stream_hup, + \&PublicInbox::Daemon::tcp_hup, + "stream_hup and tcp_hup are different on \$^O=$^O"; +} + +done_testing; diff --git a/t/psgi_log.psgi b/t/psgi_log.psgi new file mode 100644 index 000000000..ddecb4be5 --- /dev/null +++ b/t/psgi_log.psgi @@ -0,0 +1,16 @@ +#!/usr/bin/perl -w +# Copyright (C) all contributors +# License: GPL-3.0+ +# Usage: plackup [OPTIONS] /path/to/this/file +use v5.12; +use PublicInbox::WWW; +use Plack::Builder; +my $www = PublicInbox::WWW->new; +$www->preload; +builder { + enable 'AccessLog::Timed', + logger => sub { syswrite(STDOUT, $_[0]) }, + format => '%t "%r" %>s %b %D'; + enable 'Head'; + sub { $www->call($_[0]) } +} diff --git a/t/solver_git.t b/t/solver_git.t index c45011ad8..76a0d8ab9 100644 --- a/t/solver_git.t +++ b/t/solver_git.t @@ -8,8 +8,8 @@ require_git v2.6; use PublicInbox::ContentHash qw(git_sha); use PublicInbox::Spawn qw(run_qx which); use File::Path qw(remove_tree); -use PublicInbox::IO qw(write_file); -use autodie qw(close mkdir open rename symlink unlink); +use PublicInbox::IO qw(write_file try_cat); +use autodie qw(close kill mkdir open read rename symlink unlink); require_mods qw(DBD::SQLite Xapian); require PublicInbox::SolverGit; my $rdr = { 2 => \(my $null) }; @@ -476,8 +476,53 @@ EOF my $env = { PI_CONFIG => $cfgpath, TMPDIR => $tmpdir }; xsys_e $cp, qw(-Rp), $binfoo, $gone_repo; # for test_httpd $client + my $has_log; + SKIP: { # some distros may split out this middleware + require_mods 'Plack::Middleware::AccessLog::Timed', 1; + $has_log = $env->{psgi_file} = 't/psgi_log.psgi'; + } test_httpd($env, $client, 7, sub { SKIP: { + my $td_pid = $PublicInbox::TestCommon::CURRENT_DAEMON->{pid}; + my $lis = $PublicInbox::TestCommon::CURRENT_LISTENER; + kill 'STOP', $td_pid; + my @req = ("GET /$name/69df7d5/s/ HTTP/1.0\r\n", "\r\n"); + my $c0 = tcp_connect($lis); + print $c0 $req[0]; + my @c = map { + my $c = tcp_connect($lis); + print $c @req; + $c; + } (1..30); + close $_ for @c[(1..29)]; # premature disconnects + kill 'CONT', $td_pid; + read $c[0], my $buf, 16384; + print $c0 $req[1]; + read $c0, $buf, 16384; + + if ($has_log) { + # kick server to ensure all CBs are handled, first + $c0 = tcp_connect($lis); + print $c0 <= 2, 'got at least two 200 codes' or + diag explain(\%counts); + is $counts{499} + $counts{200}, 31, + 'all 1.0 connections logged for disconnects'; + } + require_cmd('curl', 1) or skip 'no curl', 1; mkdir "$tmpdir/ext"; my $rurl = "$ENV{PLACK_TEST_EXTERNALSERVER_URI}/$name"; -- 2.47.3