Hello!

On Fri, Sep 16, 2022 at 09:03:38PM +0400, Sergey Kandaurov wrote:

> > On 7 Sep 2022, at 05:46, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Tue, Sep 06, 2022 at 05:52:51PM +0400, Sergey Kandaurov wrote:
> >
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet@nginx.com>
> >> # Date 1662472340 -14400
> >> # Tue Sep 06 17:52:20 2022 +0400
> >> # Node ID 7eb87eaf06f4b9161cfe298f195947dbddedd01d
> >> # Parent 78fe648d54a79822a72f3a5f8cc79651b3b1f5a7
> >> Tests: fixed try_run() to remove temporary directory on failure.
> >>
> >> Keeping the file handle pointing to the "stderr" file prevented removal
> >> of the temporary directory on win32 if execution aborted in run().
> >> The fix is to move safe file operations surrounding run() out of the
> >> eval block such that the state of file handles is consistent.
> >> Fixes a1874249496d.
> >
> > I believe it might need some additional emphasis that this is
> > about Windows only. For example:
> >
> > Tests: fixed try_run() to remove temporary directory on win32.
> >
>
> Agree, thx.
>
> >>
> >> diff -r 78fe648d54a7 -r 7eb87eaf06f4 lib/Test/Nginx.pm
> >> --- a/lib/Test/Nginx.pm Tue Aug 30 17:24:16 2022 +0400
> >> +++ b/lib/Test/Nginx.pm Tue Sep 06 17:52:20 2022 +0400
> >> @@ -290,8 +290,9 @@ sub has_daemon($) {
> >> sub try_run($$) {
> >> my ($self, $message) = @_;
> >>
> >> + open OLDERR, ">&", \*STDERR;
> >> +
> >> eval {
> >> - open OLDERR, ">&", \*STDERR;
> >> open NEWERR, ">", $self->{_testdir} . '/stderr'
> >> or die "Can't open stderr: $!";
> >> close STDERR;
> >> @@ -299,10 +300,10 @@ sub try_run($$) {
> >> close NEWERR;
> >>
> >> $self->run();
> >> + };
> >>
> >> - close STDERR;
> >> - open STDERR, ">&", \*OLDERR;
> >> - };
> >> + close STDERR;
> >> + open STDERR, ">&", \*OLDERR;
> >>
> >> return $self unless $@;
> >
> > Wouldn't it be better to move all stderr handling out of the eval?
> >
> > diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm
> > --- a/lib/Test/Nginx.pm
> > +++ b/lib/Test/Nginx.pm
> > @@ -291,14 +291,13 @@ sub try_run($$) {
> > my ($self, $message) = @_;
> >
> > open OLDERR, ">&", \*STDERR;
> > + open NEWERR, ">", $self->{_testdir} . '/stderr'
> > + or die "Can't open stderr: $!";
> > + close STDERR;
> > + open STDERR, ">&", \*NEWERR;
> > + close NEWERR;
> >
> > eval {
> > - open NEWERR, ">", $self->{_testdir} . '/stderr'
> > - or die "Can't open stderr: $!";
> > - close STDERR;
> > - open STDERR, ">&", \*NEWERR;
> > - close NEWERR;
> > -
> > $self->run();
> > };
> >
>
> The reason was to continue hide '/stderr' open errors in eval,
> but I tend to think that it's rather better to die explicitly,
> as that's not something to happen.
>
> >
> > Alternatively, it might be the right time to backout a1874249496d
> > completely and rely on "-e ..." instead, given that it was
> > introduced in 1.19.5 and already present in all supported
> > versions.
>
> I thought about that, looks like the time has come to cleanup things.
> It doesn't break out nicely into two pieces, so comes in one change.
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1663347154 -14400
> # Fri Sep 16 20:52:34 2022 +0400
> # Node ID 16c3e0b1c8878bb127425b4da873f8eba604c879
> # Parent 78fe648d54a79822a72f3a5f8cc79651b3b1f5a7
> Tests: updated try_run() to rely on nginx "-e".
>
> The "-e" command line option introduced in nginx 1.19.5 is now used
> to print error line on startup failures with TEST_NGINX_VERBOSE set.
> This change replaces a previous approach (a1874249496d) compatible
> with pre-1.19.5 nginx versions that used to redirect stderr to file.
> Hence, "-e" compatibility is removed.
>
> As a side effect, this fixes temporary directory removal on win32
> left on startup failures because the "stderr" file was kept open.
>
> diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm
> --- a/lib/Test/Nginx.pm
> +++ b/lib/Test/Nginx.pm
> @@ -48,8 +48,6 @@ sub new {
> )
> or die "Can't create temp directory: $!\n";
> $self->{_testdir} =~ s!\\!/!g if $^O eq 'MSWin32';
> - mkdir "$self->{_testdir}/logs"
> - or die "Can't create logs directory: $!\n";
>
> Test::More::BAIL_OUT("no $NGINX binary found")
> unless -x $NGINX;
> @@ -291,24 +289,16 @@ sub try_run($$) {
> my ($self, $message) = @_;
>
> eval {
> - open OLDERR, ">&", \*STDERR;
> - open NEWERR, ">", $self->{_testdir} . '/stderr'
> - or die "Can't open stderr: $!";
> - close STDERR;
> - open STDERR, ">&", \*NEWERR;
> - close NEWERR;
> -
> + open OLDERR, ">&", \*STDERR; close STDERR;
> $self->run();
> -
> - close STDERR;
> open STDERR, ">&", \*OLDERR;
> };
>
> return $self unless $@;
>
> if ($ENV{TEST_NGINX_VERBOSE}) {
> - open F, '<', $self->{_testdir} . '/stderr'
> - or die "Can't open stderr: $!";
> + open F, '<', $self->{_testdir} . '/error.log'
> + or die "Can't open error.log: $!";
> log_core($_) while (<F>);
> close F;
> }
> @@ -350,10 +340,8 @@ sub run(;$) {
> my @globals = $self->{_test_globals} ?
> () : ('-g', "pid $testdir/nginx.pid; "
> . "error_log $testdir/error.log debug;");
> - my @error = $self->has_version('1.19.5') ?
> - ('-e', 'error.log') : ();
> exec($NGINX, '-p', "$testdir/", '-c', 'nginx.conf',
> - @error, @globals)
> + '-e', 'error.log', @globals)
> or die "Unable to exec(): $!\n";
> }
>
> @@ -425,10 +413,8 @@ sub dump_config() {
> my @globals = $self->{_test_globals} ?
> () : ('-g', "pid $testdir/nginx.pid; "
> . "error_log $testdir/error.log debug;");
> - my @error = $self->has_version('1.19.5') ?
> - ('-e', 'error.log') : ();
> my $command = "$NGINX -T -p $testdir/ -c nginx.conf "
> - . join(' ', @error, @globals);
> + . join(' ', '-e', 'error.log', @globals);

This should be changed to explicitly use "-e error.log",
much like "-c nginx.conf" above.

>
> return qx/$command 2>&1/;
> }
> @@ -481,10 +467,8 @@ sub reload() {
> my @globals = $self->{_test_globals} ?
> () : ('-g', "pid $testdir/nginx.pid; "
> . "error_log $testdir/error.log debug;");
> - my @error = $self->has_version('1.19.5') ?
> - ('-e', 'error.log') : ();
> system($NGINX, '-p', $testdir, '-c', "nginx.conf",
> - '-s', 'reload', @error, @globals) == 0
> + '-s', 'reload', '-e', 'error.log', @globals) == 0
> or die "system() failed: $?\n";
>
> } else {
> @@ -506,10 +490,8 @@ sub stop() {
> my @globals = $self->{_test_globals} ?
> () : ('-g', "pid $testdir/nginx.pid; "
> . "error_log $testdir/error.log debug;");
> - my @error = $self->has_version('1.19.5') ?
> - ('-e', 'error.log') : ();
> system($NGINX, '-p', $testdir, '-c', "nginx.conf",
> - '-s', 'quit', @error, @globals) == 0
> + '-s', 'quit', '-e', 'error.log', @globals) == 0
> or die "system() failed: $?\n";
>
> } else {
> @@ -530,10 +512,8 @@ sub stop() {
> my @globals = $self->{_test_globals} ?
> () : ('-g', "pid $testdir/nginx.pid; "
> . "error_log $testdir/error.log debug;");
> - my @error = $self->has_version('1.19.5') ?
> - ('-e', 'error.log') : ();
> system($NGINX, '-p', $testdir, '-c', "nginx.conf",
> - '-s', 'stop', @error, @globals) == 0
> + '-s', 'stop', '-e', 'error.log', @globals) == 0
> or die "system() failed: $?\n";
>
> } else {

Otherwise looks good.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org