Hello!
On Wed, Mar 29, 2023 at 04:05:20AM +0400, Sergey Kandaurov wrote:
>
> > On 22 Mar 2023, at 05:02, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Mon, Mar 20, 2023 at 06:13:34PM +0400, Sergey Kandaurov wrote:
> >
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet@nginx.com>
> >> # Date 1679321601 -14400
> >> # Mon Mar 20 18:13:21 2023 +0400
> >> # Node ID f3225ad9300ee2c11c0dec54b9605e67060b7347
> >> # Parent 1e1d0f3874b0c5b1e399ec76b0198b5c9c265a36
> >> Tests: run syslog and error_log tests on win32.
> >>
> >> They are supposed to work well now, no reason to skip them.
> >
> > It might be a good idea to explicitly mention why it wasn't
> > supposed to work.
>
> It wasn't supposed to work due to stderr tests, now fixed.
> See full description below in the updated series.
Not really. It was originally disabled because the test uses
shared memory, which wasn't functional on Windows with ASLR till
nginx 1.9.0.
changeset: 334:c0bd624db067
user: Maxim Dounin <mdounin@mdounin.ru>
date: Mon Sep 16 23:55:04 2013 +0400
summary: Tests: disable error_log.t on win32 as it uses shared memory.
Since nginx 1.9.0 the test can be enabled, assuming other issues,
such as stderr tests, are fixed.
> >
> >> An exception is logging to stderr.
> >
> > And why stderr is not supposed to.
> >
> > (Just in case, the culprit is CreateProcess(CREATE_NO_WINDOW) used
> > when starting worker processes, which creates a process without a
> > console. This probably can be improved to preserve console if
> > logging to stderr is used.)
>
> Thanks, applied to description.
>
> >
> >>
> >> diff --git a/error_log.t b/error_log.t
> >> --- a/error_log.t
> >> +++ b/error_log.t
> >> @@ -22,8 +22,6 @@ use Test::Nginx;
> >> select STDERR; $| = 1;
> >> select STDOUT; $| = 1;
> >>
> >> -plan(skip_all => 'win32') if $^O eq 'MSWin32';
> >> -
> >> my $t = Test::Nginx->new()->has(qw/http limit_req/)
> >> ->plan(25)->write_file_expand('nginx.conf', <<'EOF');
> >>
> >> @@ -110,7 +108,7 @@ EOF
> >> open OLDERR, ">&", \*STDERR;
> >> open STDERR, '>', $t->testdir() . '/stderr' or die "Can't reopen STDERR: $!";
> >> open my $stderr, '<', $t->testdir() . '/stderr'
> >> - or die "Can't open stderr file: $!";
> >> + or die "Can't open stderr file: $!" unless $^O eq 'MSWin32';
> >
> > It shouldn't be a problem to open the file on Windows. Though
> > will require explicitly closing it at the end of the test, so the
> > test suite will be able to remove the test directory in
> > destructor.
> >
> > (In other tests this will also require reordering of daemon
> > startup, to avoid leaking the descriptor to daemons.)
>
> Applied too.
>
> >> [..]
> >> @@ -255,13 +259,16 @@ sub syslog_daemon {
> >> LocalAddr => "127.0.0.1:$port"
> >> );
> >>
> >> - open my $fh, '>', $t->testdir() . '/' . $file;
> >> - select $fh; $| = 1;
> >> + my $path = $t->testdir() . '/' . $file;
> >> + open my $fh, '>', $path;
> >> + close $fh;
> >>
> >> while (1) {
> >> my $buffer;
> >> $s->recv($buffer, 4096);
> >> + open $fh, '>>', $path;
> >> print $fh $buffer . "\n";
> >> + close $fh;
> >> }
> >
> > It might be a good idea to clarify in the commit log why this is
> > needed. (Notably, that pseudo-processes created by Perl's fork()
> > emulation on win32 cannot be gracefully stopped when they are
> > blocked in a system call, and using kill('KILL') as nginx does
> > seems to leave the files open.)
>
> Well, I don't like this kind of open/close tango.
> Reading syslog can be done in the main process, without
> invoking pseudo-processes with their limitations.
> This should work with minor number of syslog messages
> as seen in tests. Below is updated series, syslog part
> is extracted into a separate change.
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1680046138 -14400
> # Wed Mar 29 03:28:58 2023 +0400
> # Node ID be1832cdf571d465aed741b7dcbd583cedc43365
> # Parent 0351dee227a8341e442feeb03920a46b259adeb5
> Tests: revised reading syslog messages with test daemon.
>
> On win32, terminating test daemons blocked by a system call leads
> to leaking file descriptors, which breaks temp directory removal.
> See limitations of pseudo-processes used in Perl's fork() emulation.
> Now it is replaced with reading syslog in the main process.
>
> diff --git a/mail_error_log.t b/mail_error_log.t
> --- a/mail_error_log.t
> +++ b/mail_error_log.t
> @@ -90,12 +90,16 @@ open my $stderr, '<', $t->testdir() . '/
> or die "Can't open stderr file: $!";
>
> $t->run_daemon(\&Test::Nginx::IMAP::imap_test_daemon);
> -$t->run_daemon(\&syslog_daemon, port(8981), $t, 's_glob.log');
> -$t->run_daemon(\&syslog_daemon, port(8982), $t, 's_info.log');
> +
> +my ($s_glob, $s_info) = map {
> + IO::Socket::INET->new(
> + Proto => 'udp',
> + LocalAddr => '127.0.0.1:' . port($_)
> + )
> + or die "Can't open syslog socket: $!";
> +} 8981, 8982;
>
> $t->waitforsocket('127.0.0.1:' . port(8144));
> -$t->waitforfile($t->testdir . '/s_glob.log');
> -$t->waitforfile($t->testdir . '/s_info.log');
>
> $t->run();
>
> @@ -124,16 +128,16 @@ is(lines($t, 'stderr', '[debug]'), 0, 's
>
> like($t->read_file('e_glob.log'), qr!nginx/[.0-9]+!, 'error global');
> like($t->read_file('e_glob2.log'), qr!nginx/[.0-9]+!, 'error global 2');
> -is_deeply(levels($t, 'e_glob.log'), levels($t, 'e_glob2.log'),
> - 'multiple error global');
> +is_deeply(levels($t->read_file('e_glob.log')),
> + levels($t->read_file('e_glob2.log')), 'multiple error global');
>
> # syslog
>
> parse_syslog_message('syslog', get_syslog());
>
> -is_deeply(levels($t, 's_glob.log'), levels($t, 'e_glob.log'),
> +is_deeply(levels(read_syslog($s_glob)), levels($t->read_file('e_glob.log')),
> 'global syslog messages');
> -is_deeply(levels($t, 's_info.log'), levels($t, 'e_info.log'),
> +is_deeply(levels(read_syslog($s_info)), levels($t->read_file('e_info.log')),
> 'mail syslog messages');
>
> ###############################################################################
> @@ -153,10 +157,10 @@ sub lines {
> }
>
> sub levels {
> - my ($t, $file) = @_;
> + my ($file) = @_;
> my %levels_hash;
>
> - map { $levels_hash{$_}++; } ($t->read_file($file) =~ /(\[\w+\])/g);
> + map { $levels_hash{$_}++; } ($file =~ /(\[\w+\])/g);
>
> return \%levels_hash;
> }
> @@ -193,6 +197,19 @@ sub get_syslog {
> return $data;
> }
>
> +sub read_syslog {
> + my ($s) = @_;
> + my $data = '';
> +
> + IO::Select->new($s)->can_read(1);
> + while (IO::Select->new($s)->can_read(0.1)) {
> + my $buffer;
> + sysread($s, $buffer, 4096);
> + $data .= $buffer;
> + }
> + return $data;
> +}
> +
> sub parse_syslog_message {
> my ($desc, $line) = @_;
>
> @@ -246,23 +263,3 @@ SKIP: {
> }
>
> ###############################################################################
> -
> -sub syslog_daemon {
> - my ($port, $t, $file) = @_;
> -
> - my $s = IO::Socket::INET->new(
> - Proto => 'udp',
> - LocalAddr => "127.0.0.1:$port"
> - );
> -
> - open my $fh, '>', $t->testdir() . '/' . $file;
> - select $fh; $| = 1;
> -
> - while (1) {
> - my $buffer;
> - $s->recv($buffer, 4096);
> - print $fh $buffer . "\n";
> - }
> -}
> -
> -###############################################################################
Well, I can't say I like this approach. While there are some
issues with pseudo-processes on win32, in general test daemons are
much easier to read and understand than using some socket
intermixed with the test code.
Also note that the particular approach taken implies noticeably
more per-test code, such as in:
-is_deeply(levels($t, 's_glob.log'), levels($t, 'e_glob.log'),
+is_deeply(levels(read_syslog($s_glob)), levels($t->read_file('e_glob.log')),
While probably this can be optimized away (either by moving
read_syslog() into levels(), or by calling it separately to save
data to the file and then using levels() unmodified), it might be
easier to keep the existing code with test daemons instead.
If the open()/write()/close() approach makes you feel
uncomfortable, abstracting it into a function might be the way to
go. Something similar to $t->write_file() but with append should
do the trick.
[...]
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
