git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH v7 1/9] gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_error

From
Jakub Narebski <jnareb@gmail.com>
Date
Dec 26, 2010, 22:25 UTC
Message-ID
<201012262325.51585.jnareb@gmail.com>
In-Reply-To
<20101226095054.GB21588@burratino>
On Sun, 26 Dec 2010, Jonathan Nieder wrote:
> Jakub Narebski wrote:
>> On Thu, 23 Dec 2010, Jonathan Nieder wrote:
Show 8 quoted lines
>>> die_error gets called when server load is too high; I wonder whether
>>> it is right to go back for another request in that case.
>>
>> If client (web browser) are requesting connection, we have to tell it
>> something anyway.
> 
> Right, I should have thought a few seconds more.  Respawning
> gitweb.perl would generate _more_ load[1].
Show 11 quoted lines
> [1] I can imagine scenarios in which exiting gitweb would help
> alleviate the load, involving:
> 
>  - large memory footprint for each gitweb process forcing the system
>    into swapping (e.g., from a memory leak), or
>  - FastCGI-like server noticing the load and choosing to decrease the
>    number of gitweb instances.
> 
> In the usual case, presumably gitweb memory footprint is small and
> FastCGI-like servers limit the number of gitweb instances to a modest
> fixed number.
I assume that CGI / FastCGI / mod_perl (+ ModPerl::Registry) web server
would know how to regulate number of workers according to the server
load.
Show 7 quoted lines
>>> A broken per-request (or other) configuration could potentially leave
>>> a gitweb process in a broken state,
> [...]
>> 'die $@ if $@' would call CORE::die, which means it would end gitweb
>> process.
> 
> This is referring to a later patch?
I'm sorry I haven't made myself clear.
What I meant here is that gitweb includes the following code
	if (-e $GITWEB_CONFIG) {
		do $GITWEB_CONFIG;
		die $@ if $@;
	}

which means that CGI::Carp::die is called, which might call handle_errors_html, and which ends in CORE::die, which ends gitweb process. So if there is no way for broken configuration to leave gitweb in a rboken state _at this point in series_.

Thank you for thinking about this, because it could cause problems (could because I have not checked if it does or if it doesn't) in the following patch, when gitweb uses eval / die for error handling. Then it might happen when $per_request_config is false or CODE that instead of trying to reread broken config on subsequent requests, we will run with broken config. It depends if "die"-ing in evaluate_gitweb_config would prevent setting $first_request to false. I'd have to check that.

Show 18 quoted lines
>> For CGI server it doesn't matter anyway, as for each request the process
>> is respawned anyway (together with respawning Perl interpreter), and I
>> think that ModPerl::Registry and FastCGI servers monitor process that it
>> is to serve requests, and respawn it if/when it dies.
> 
> Sorry, that was unclear of me.  I meant that buggy configuration could
> leave a gitweb process in buggy but alive state and frequent failing
> requests might be a way to notice that.  Contrived example (just to
> illustrate what I mean):
> 
> 	our $version .= ".custom";
> 	if (length $version>= 1000) {	# untested, buggy code goes here.
> 		@diff_opts = ("--nonsense");
> 	}
> 
> I think I was not right to worry about this, either.  It is better to
> make such unusual and buggy configurations as noticeable as possible
> so they can be fixed.
See above.
Show 14 quoted lines
> [...]
>> But actually handle_errors_html gets called only from fatalsToBrowser,
>> which in turn gets called from CGI::Carp::die... which ends calling
>> CODE::die (aka realdie), which ends CGI process anyway.
>>
>> That is why die_error ends with
>> 
>>	goto DONE_GITWEB
>>		unless ($opts{'-error_handler'});
>> 
>> i.e. it doesn't goto DONE_GITWEB nor DONE_REQUEST if called from
>> handle_errors_html anyway.
> [...]
>> Thanks a lot for your comments.
Which should make it in either commit message, or comments, I guess.
 
> Thanks for a thorough explanation.  For what it's worth, with or
> without removal of the DONE_GITWEB: label,
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
-- 
Jakub Narebski
Poland
Previous: Jonathan NiederNext: Jakub Narebski
Message 11 of 34 in “gitweb: Output caching, with eval/die based error handling”
  1. 0/9 gitweb: Output caching, with eval/die based error handlingJakub Narebski, Dec 22, 2010
  2. 1/9 gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_errorJakub Narebski, Dec 22, 2010
  3. Jonathan NiederDec 23, 2010
  4. Jakub NarebskiDec 25, 2010
  5. diff: funcname and word patterns for perlJonathan Nieder, Dec 26, 2010
  6. Jakub NarebskiDec 26, 2010
  7. Junio C HamanoDec 27, 2010
  8. Jakub NarebskiDec 27, 2010
  9. Jeff KingDec 28, 2010
  10. Jonathan NiederDec 26, 2010
  11. Jakub NarebskiDec 26, 2010
  12. 2/9 gitweb: use eval + die for error (exception) handlingJakub Narebski, Dec 22, 2010
  13. Jonathan NiederDec 23, 2010
  14. Jakub NarebskiDec 25, 2010
  15. 5/9 gitweb: Make die_error just die, and use send_error to create error pagesJakub Narebski, Jan 4, 2011
  16. 3/9 gitweb: Introduce %actions_info, gathering information about actionsJakub Narebski, Dec 22, 2010
  17. 4/9 gitweb: Prepare for splitting gitwebJakub Narebski, Dec 22, 2010
  18. Jonathan NiederDec 24, 2010
  19. Jakub NarebskiDec 26, 2010
  20. 5/9 t/test-lib.sh: Export also GIT_BUILD_DIR in test_externalJakub Narebski, Dec 22, 2010
  21. 6/9 gitweb/lib - Simple output capture by redirecting STDOUT to fileJakub Narebski, Dec 22, 2010
  22. Jonathan NiederDec 24, 2010
  23. Jakub NarebskiDec 26, 2010
  24. 7/9 gitweb/lib - Very simple file based cacheJakub Narebski, Dec 22, 2010
  25. 8/9 gitweb/lib - Cache captured output (using compute_fh)Jakub Narebski, Dec 22, 2010
  26. 9/9 gitweb: Add optional output cachingJakub Narebski, Dec 22, 2010
  27. 10/9 gitweb: Background cache generation and progress indicatorJakub Narebski, Dec 31, 2010
  28. 11/9 [PoC] gitweb/lib - tee, i.e. print and capture during cache entry generationJakub Narebski, Jan 3, 2011
  29. J.H.Jan 3, 2011
  30. Jakub NarebskiJan 4, 2011
  31. Jakub NarebskiJan 4, 2011
  32. 11/9 [PoC] gitweb/lib - HTTP-aware output cachingJakub Narebski, Jan 5, 2011
  33. Jonathan NiederDec 26, 2010
  34. Jonathan NiederDec 26, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.