Re: [PATCH 18/18] gitweb: Add better error handling for gitweb caching

[Jakub Narebski <jnareb@gmail.com>]

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

> This basically finishes the plumbing for caching the error pages
> as the are generated.
> 
> If an error is hit, create a <hash>.err file with the error.  This
> will interrupt all currently waiting processes and they will display
> the error, without any additional refreshing.
> 
> On a new request a generation will be attempted, should it succed the
> <hash.err> file is removed (if it exists).

Could you split 17 and 18 patches slightly differently, at least not
using variables which were not declared first?
 
> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>

Hmmm... I certainly hope that this complication is not really needed.
I have trouble following code flow (no comments), so I'd try to do
fresh review again tomorrow.

> ---
>  gitweb/gitweb.perl  |    8 ++++++++
>  gitweb/lib/cache.pl |   14 ++++++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d39982a..5a9660a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -41,6 +41,7 @@ our $version = "++GIT_VERSION++";
>  
>  # Output buffer variable
>  our $output = "";
> +our $output_err = "";

It is used by earlier patches, and declared only there.

>  
>  our ($my_url, $my_uri, $base_url, $path_info, $home_link);
>  sub evaluate_uri {
> @@ -303,6 +304,9 @@ our $fullhashpath = *STDOUT;
>  our $fullhashbinpath = *STDOUT;
>  our $fullhashbinpathfinal = *STDOUT;
>  
> +our $cacheErrorCache = 0; # false

$capture_error_output, isn't it?

> +our $cacheErrorCount = 0;

$cached_error_count, or something like that, isn't it?

> +
>  our $full_url;
>  our $urlhash;
>  
> @@ -3786,6 +3790,7 @@ sub die_error {
>  	# Reset the output so that we are actually going to STDOUT as opposed
>  	# to buffering the output.
>  	reset_output() if ($cache_enable && ! $cacheErrorCache);
> +	$cacheErrorCount++ if( $cacheErrorCache );

Where it is decremented?  A comment, if you please.

>  
>  	git_header_html($http_responses{$status}, undef, %opts);
>  	print <<EOF;
> @@ -3801,6 +3806,9 @@ EOF
>  	print "</div>\n";
>  
>  	git_footer_html();
> +
> +	die_error_cache($output) if ( $cacheErrorCache );
> +

That's cache_die_error_output, or something like that, isn't it?

It's hard to review this patch when die_error_cache is defined in
separate (previous) patch.

>  	goto DONE_GITWEB
>  		unless ($opts{'-error_handler'});
>  }
> diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
> index 6cb82c8..2e7ca69 100644
> --- a/gitweb/lib/cache.pl
> +++ b/gitweb/lib/cache.pl
> @@ -240,8 +240,14 @@ sub cacheUpdate {
>  	# Trap all output from the action
>  	change_output();
>  
> +	# Set the error handler so we cache
> +	$cacheErrorCache = 1; # true
> +
>  	$actions{$action}->();
>  
> +	# Reset Error Handler to not cache
> +	$cacheErrorCache = 0; # false
> +
>  	# Reset the outputs as we should be fine now
>  	reset_output();
>  
> @@ -295,6 +301,8 @@ sub cacheUpdate {
>  		close($cacheFileBG);
>  	}
>  
> +	unlink("$fullhashpath.err") if (-e "$fullhashpath.err");
> +
>  	if ( $areForked ){
>  		exit(0);
>  	} else {
> @@ -339,6 +347,9 @@ sub cacheWaitForUpdate {
>  	my $max = 10;
>  	my $lockStat = 0;
>  
> +	# Call cacheDisplayErr - if an error exists it will display and die.  If not it will just return
> +	cacheDisplayErr($action);
> +
>  	if( $backgroundCache ){
>  		if( -e "$fullhashpath" ){
>  			open($cacheFile, '<:utf8', "$fullhashpath");
> @@ -402,6 +413,9 @@ EOF
>  		close($cacheFile);
>  		$x++;
>  		$combinedLockStat = $lockStat;
> +
> +		# Call cacheDisplayErr - if an error exists it will display and die.  If not it will just return
> +		cacheDisplayErr($action);
>  	} while ((! $combinedLockStat) && ($x < $max));
>  	print <<EOF;
>  </body>
> -- 
> 1.7.2.3
> 

-- 
Jakub Narebski
Poland
ShadeHawk on #git