Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Dec 10, 2010, 01:49 UTC
- Message-ID
- <m3r5dqz9c5.fsf@localhost.localdomain>
- In-Reply-To
- <1291931844-28454-18-git-send-email-warthog9@eaglescrag.net>
"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:
Show 7 quoted lines
> 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?
Show 5 quoted lines
> > 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???
Show 19 quoted lines
>
> 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.
Show 9 quoted lines
> + exit(0);
> + }else{
> + return;
> + }
> + }
> +
> + # Actually dump the output to the proper file handler
> + local $/ = undef;
> + $|++;Why not
+ local $| = 1;
Show 5 quoted lines
> + print $cacheFileErr "$output"; > + $|--; > + > + flock($cacheFileErr,LOCK_UN); > + close($cacheFileErr);
Closing file will unlock it.
Show 5 quoted lines
> +
> + if ( $areForked ){
> + exit(0);
> + }else{
> + return;So die_error_cache would not actually work like "die" here and like die_error(), isn't it?
Show 21 quoted lines
> + }
> +}
> +
>
> 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
Show 8 quoted lines
> + "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