Re: [PATCH 18/18] gitweb: Add better error handling for gitweb caching
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Dec 10, 2010, 01:56 UTC
- Message-ID
- <m3mxoez90s.fsf@localhost.localdomain>
- In-Reply-To
- <1291931844-28454-19-git-send-email-warthog9@eaglescrag.net>
"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:
Show 9 quoted lines
> 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.
Show 14 quoted lines
> --- > 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.
Show 8 quoted lines
>
> 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?
Show 9 quoted lines
> +
> 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.
Show 10 quoted lines
>
> 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.
Show 54 quoted lines
> 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