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

On Fri, 10 Dec 2010, J.H. wrote:

Well, if you, the author, cannot follow code flow of your own code, what
does it matter for being sure that this code is bug free?  What does this
matter for maintability of this code?
 

That rant aside, error / exception handling in gitweb is currently not fitting well with output caching, at least the locking one.

die_error() functions as a kind of exception handling; we rely that on the fact that calling die_error() would end request, independent on how deep in the stack we are. Originally die_error() ended with 'exit', which ModPerl::Registry redefined for it to end request and not exit worker. Then 'exit' was replaced by 'goto DONE_GITWEB' to jump out of several levels of calls; I didn't know then about ModPerl::Registry redefining 'exit'... and actually it should be 'goto DONE_REQUEST', like in "[PATCH/RFC] gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_error"

  http://permalink.gmane.org/gmane.comp.version-control.git/162156

It is because die_error is exception mechanism, and in current incarnation always ends request, that is why error pages (generated by die_error) were not cached: we jump out of capturing and out of caching. The additional reasoning is that we don't want to bloat cache with error pages, which IMHO usually gets requested only once (assumption: different clients makes different errors).

Now in most cases the approach taken to modify die_error for caching only by adding explicit turning off capturing at the beginning of die_error is enough.

1. Single client, no generating in background (note that if given URL is
   never cached, we would not invoke background generation to refresh shown
   stale data - there wouldn't be stale data).

   In this case if there is an expected error, die_error() gets explicitely
   invoked, turns off capturing, prints error page to client, and ends
   request.

   In the case of uncaught "die", it would be caught by CGI::Carp error
   handler, and passed to handle_errors_html() subroutine (thanks to gitweb
   using set_message(\&handle_errors_html)), which runs die_error() with
   options making it not print HTTP header (which was already printed by
   CGI::Carp error handler), and not exit - the CGI::Carp error handler
   would end request instead.  die_error() turns of capturing, prints
   error page, and CGI::Carp error handler ends request.

2. Two clients arriving at exactly the same error (same link), at the
   same time.  This is quite unlikely.

   In my rewrite there is loop in ->compute method in rewritten caching
   engine, which reads:

      do {
          ...
      } until (<received data to show to user> || <tried to generate data ourself>);

   This means that one client acquires writers lock, die_error prints error
   page and exists, second client notices that it didn't get anything but
   didn't try it itself yet, and dies itself on die_error()

   Dealing with "die"-ing works the same as in the case described above,
   so there is no problem from this area neither.

   Alternate solution would be to treat it as the case described below.

3. Gitweb runs generating cache entry in background.  Note that if error
   pages are not cached, there would be no stale pages to serve while 
   regenerating data in background - so entering background process can
   be done only thanks to "Generating..." page.
   
   We can try _prevent this from happening_, as I did in my rewrite by
   introducing initial/startup delay in "Generating..." (which has also
   other reasons to use), or via 'generating_info_is_safe' protection.

   Otherwise we need to pass error page from background process to
   foreground proces that is showing "Generating..." page; well, to be
   more exact, with current workings of "Generating..." it would be its
   successor (next request, after reload / refresh).

   Note: the fact that it is *next request* that needs an error page
   (otherwise we would show "Generating..." page yet again).

   So what die_error needs to do if it finds itself in the background
   process (perhaps explicit $background boolean variable, perhaps
   comparing $$ with $my_pid, perhaps checking if STDOUT is closed)
   it needs to somehow write cache entry, perhaps in a special way
   marking it as error page.  The problem is to do it in generic way,
   that would not make it impossible to use other caching engine, or
   other capturing engine, in the future.

   Note also that at the end of background process (perhaps at the
   end of die_error) we need to exit process, and not just end request,
   so we should use 'CORE::exit(0);'.

The problem with 3rd case makes me think that it is high time that die_error use Perl 5 exception throwing and handling mechanism, namely "die" (for throwing errors, to be used in die_error), and "eval BLOCK" (to catch errors).

As proposed on #perl channel when asking about this situation, die_error would use 'die \$DIE_ERROR' to throw reference, or throw an object, to easy distinguish between handled error from die_error, and unhandled error from Perl (where we assume that all errors are strings).

run_request() or run() would then use 'eval { ... }', which has the additional advantage that we can get rid of CGI::Carp::set_message, which doesn't allow to use custom HTTP status, and supposedly doesn't work with mod_perl 2.0. Instead of adding capture_stop() to die_error(), the capture mechanism should use 'eval { ... }', and just print response if there was exception (like Capture::Tiny does)... or return captured error page to be cached in the case of being in background process.

Well, any way we choose to handle it, the code should be very clear, and handle all cases (other caching engines, perhaps also other capture engines, non-persistent and persistent environments, redefined 'exit' like in ModPerl::Registry case, not redefined 'exit' like I think in FastCGI case, etc., etc.).

[...]

Yes, please.  Splitting those changes into 17 & 18 didn't make it more
clear (usually smaller commit == easier to review), but rather less
transparent.
 

Or you can use 'write to File::Temp::tempfile, rename file' trick for atomic update to file. I use it in early commits in my rewrite of gitweb caching, see e.g.:

  http://repo.or.cz/w/git/jnareb-git.git/blob/refs/heads/gitweb/cache-kernel-v6:/gitweb/lib/GitwebCache/SimpleFileCache.pm#l362
 

Global variables, action at considerable distance.
 

Note the difference between exit and Core::exit, when running gitweb
from mod_perl using ModPerl::Registry handler.
 

O.K., but please note that I have found LOCK_UN to be unreliable.
 

By the way, why do you fork indiscriminately (remember that forking
is not without performance cost), even when background generation is
turned off, or you don't need background generation?
 
Wouldn't fallback on non-background generation if fork() fails, as in
my rewrite of gitweb caching series be a better solution?

Perhaps

  print while <$cacheFileErr>;

(we use it in already in "print while <$fd>;" in git_blame_common())?

Or, if we use File::Copy, perhaps File::Copy::copy($cacheFileErr, \*STDOUT);
or something like that.