[PATCH] Tests: fixed try_run() to remove temporary directory on failure

[Maxim Dounin]

Hello!

On Tue, Sep 06, 2022 at 05:52:51PM +0400, Sergey Kandaurov wrote:

> # HG changeset patch
> # User Sergey Kandaurov <pluknet at 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.

> 
> 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();
 	};
 

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.

-- 
Maxim Dounin
http://mdounin.ru/