Re: [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling
- From
J.H. <warthog9@eaglescrag.net>
- Date
- Dec 10, 2010, 08:33 UTC
- Message-ID
- <4D01E5CC.6010301@eaglescrag.net>
- In-Reply-To
- <m3r5dqz9c5.fsf@localhost.localdomain>
Show 10 quoted lines
> 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.
Show 14 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???
in breaking up the series it got included in the wrong spot, and apparently removed and re-added correctly, should be fixed in v9
Show 7 quoted lines
>> +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...
Show 13 quoted lines
>> +
>> + 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.
Show 9 quoted lines
>> + >> + # Actually dump the output to the proper file handler >> + local $/ = undef; >> + $|++; > > Why not > > + local $| = 1; >
Done.
Show 8 quoted lines
> >> + print $cacheFileErr "$output"; >> + $|--; >> + >> + flock($cacheFileErr,LOCK_UN); >> + close($cacheFileErr); > > Closing file will unlock it.
Doesn't really hurt to be explicit though.
Show 8 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?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.
Show 35 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
>
>> + "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.
Show 7 quoted lines
> >> + 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