Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling

[Jakub Narebski <jnareb@gmail.com>]

"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:

> To quote myself from an e-mail of mine:
> 
> 	I've got a hammer, it clearly solves all problems!
> 
> This is the prepatory work to set up a mechanism inside the
> caching engine to cache the error pages instead of throwing
> them straight out to the client.

There is no problem with capturing output of die_error, nor there is a
problem with caching error pages (perhaps transiently in memory).

The problem is that subroutines calling die_error assum that it would
exit ending subroutine that is responsible for generating current
action; see "goto DONE_GITWEB" which should be "goto DONE_REQUEST",
and which was "exit 0" some time ago at the end of die_error().

With caching error pages you want die_error to exit $actions{$action}->(),
but not exit cache_fetch().  How do you intend to do it?

> 
> This adds two functions:
> 
> die_error_cache() - this gets back called from die_error() so
> that the error message generated can be cached.

*How* die_error_cache() gets called back from die_error()?  I don't
see any changes to die_error(), or actually any calling sites for
die_error_cache() in the patch below.
 
> cacheDisplayErr() - this is a simplified version of cacheDisplay()
> that does an initial check, if the error page exists - display it
> and exit.  If not, return.

Errr... isn't it removed in _preceding_ patch?  WTF???

> 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> ---
>  gitweb/lib/cache.pl |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 52 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index a8c902d..6cb82c8 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -302,6 +302,36 @@ sub cacheUpdate {
>  	}
>  }
>  
> +sub die_error_cache {
> +	my ($output) = @_;
> +
> +	open(my $cacheFileErr, '>:utf8', "$fullhashpath.err");
> +	my $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);

Why do you need to lock here?  A comment would be nice.

> +
> +	if (! $lockStatus ){
> +		if ( $areForked ){

Grrrr...

But if it is here to stay, a comment if you please.

> +			exit(0);
> +		}else{
> +			return;
> +		}
> +	}
> +
> +	# Actually dump the output to the proper file handler
> +	local $/ = undef;
> +	$|++;

Why not

  +	local $| = 1;


> +	print $cacheFileErr "$output";
> +	$|--;
> +
> +	flock($cacheFileErr,LOCK_UN);
> +	close($cacheFileErr);

Closing file will unlock it.

> +
> +	if ( $areForked ){
> +		exit(0);
> +	}else{
> +		return;

So die_error_cache would not actually work like "die" here and like
die_error(), isn't it?

> +	}
> +}
> +
>  
>  sub cacheWaitForUpdate {
>  	my ($action) = @_;
> @@ -380,6 +410,28 @@ EOF
>  	return;
>  }
>  
> +sub cacheDisplayErr {
> +
> +	return if ( ! -e "$fullhashpath.err" );
> +
> +	open($cacheFileErr, '<:utf8', "$fullhashpath.err");
> +	$lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
> +
> +	if (! $lockStatus ){
> +		show_warning(
> +				"<p>".
> +				"<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".

esc_path

> +				"This is about as screwed up as it gets folks - see your systems administrator for more help with this.".
> +				"<p>"
> +				);
> +	}
> +
> +	while( <$cacheFileErr> ){
> +		print $_;
> +	}

Why not 'print <$cacheFileErr>' (list context), like in insert_file()
subroutine?

> +	exit(0);
> +}

Callsites?

Note: I have't read next commit yet.

-- 
Jakub Narebski
Poland
ShadeHawk on #git