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

["J.H." <warthog9@eaglescrag.net>]

> 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?

Well there's one bug in how that function ends in looking at it again,
basically the return case shouldn't happen, and that function should
end, like your suggesting in the first part of your question (with
respect to DONE_GITWEB)

In the second part, your not thinking with the fork() going (though in
thinking sans the fork this might not work right).

It's the background process that will call die_error in such a way that
die_error_cache will get invoked.  die_error_cache will write the .err
file out, and the whole thing should just exit.

Though now that I say that there's an obvious bug in the case where
forking didn't work at all, in that case you would get a blank page as
the connection would just be closed.  If you refreshed (say hitting F5)
you'd get the error at that point.

Need to fix that non-forked problem though.

>> 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???

in breaking up the series it got included in the wrong spot, and
apparently removed and re-added correctly, should be fixed in v9

>> +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.

At any point when a write happens there's the potential for multiple
simultaneous writes.  Locking becomes obvious, when your trying to
prevent multiple processes from writing to the same thing at the same
time...

>> +
>> +	if (! $lockStatus ){
>> +		if ( $areForked ){
> 
> Grrrr...
> 
> But if it is here to stay, a comment if you please.
> 
>> +			exit(0);
>> +		}else{
>> +			return;
>> +		}
>> +	}

The exit(0) or return have been removed in favor of DONE_GITWEB, as
we've already errored if we are broken here we should just die.

>> +
>> +	# Actually dump the output to the proper file handler
>> +	local $/ = undef;
>> +	$|++;
> 
> Why not
> 
>   +	local $| = 1;
> 

Done.

> 
>> +	print $cacheFileErr "$output";
>> +	$|--;
>> +
>> +	flock($cacheFileErr,LOCK_UN);
>> +	close($cacheFileErr);
> 
> Closing file will unlock it.

Doesn't really hurt to be explicit though.

>> +
>> +	if ( $areForked ){
>> +		exit(0);
>> +	}else{
>> +		return;
> 
> So die_error_cache would not actually work like "die" here and like
> die_error(), isn't it?

that was ejected, it was a bug.  DONE_GITWEB is more correct, though I
might need to add a hook to display the error message in the case that
the process didn't fork.

>> +	}
>> +}
>> +
>>  
>>  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
> 
>> +				"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?

I've had buffer problems with 'print <$cacheFileErr>' in some cases.
This is small enough it shouldn't happen, but I've gotten into the habit
of doing it this way.  I can change it if you like.

> 
>> +	exit(0);
>> +}
> 
> Callsites?
> 
> Note: I have't read next commit yet.

Next patch.

If you'd rather I can squash 17 & 18 into a single commit.

- John 'Warthog9' Hawley