Re: [PATCH 18/18] gitweb: Add better error handling for gitweb caching
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:50:13
"John 'Warthog9' Hawley" [off-list ref] 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.
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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