Re: [PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 12, 2010, 15:17 UTC
Message-ID: <201012121617.04997.jnareb@gmail.com>
In-Reply-To: <4D045CD6.9060806@eaglescrag.net>

On Sun, 12 Dec 2010, J.H. wrote:

Show 33 quoted lines

> > Hmm... anuthing that happens after 'use CGI::Carp;' is parsed should
> > have STDERR redirected to web server logs, see CGI::Carp manpage
> > 
> >     [...]
> >  
> >        use CGI::Carp
> > 
> >     And the standard warn(), die (), croak(), confess() and carp() calls will
> >     automagically be replaced with functions that write out nicely time-stamped
> >     messages to the HTTP server error log.
> > 
> >     [...]
> > 
> >     REDIRECTING ERROR MESSAGES
> > 
> >        By default, error messages are sent to STDERR.  Most HTTPD servers direct
> >        STDERR to the server's error log.
> > 
> >     [...]
> > 
> > Especially the second part.
> 
> That was not what I was seeing, so either something I was doing was
> horking how CGI::Carp works, or their claim that "most HTTPD server
> direct STDERR to the server's error log" is false.
> 
> > Could you give us example which causes described misbehaviour?
> 
> While I was working on the trapping of the error pages I started getting
> 500 errors when going to a non-existent sha1.  Running the command from
> the cli revealed that a message from a git command was making it out to
> the console.  Redirecting STDERR masked the error from git, and stopped
> premature data being sent out before the headers were sent.

Generally if something worked, and stopped working, don't you think that you should concentrate on fixing your code, and not papering over the issue?

The fact that "Running the command from the cli revealed that a message from a git command was making it out to the console." doesn't mean anything, because when running gitweb from commandline both stdout and stderr are redirected to terminal, by default. So you should worry only if there is premature data being sent to standard output, with standard error redirected to /dev/null (2>/dev/null).

What CGI::Carp does is (re)define 'die' and 'warn' to support
fatalsToBrowser and warningsToBrowser, and to add timestamp and other
auxiliary information: in the end 'die' calls 'CORE::die', and 'warn'
calls 'CORE::warn' - both of which write to STDERR.  This means that
warnings from git commands sent to standard error do not get timestamp
appended.  Note that standard output from git commands run by gitweb
is always captured.

Show 10 quoted lines

> > I have nothing against this patch: if you have to have it, then you
> > have to have it.  I oly try to understand what might be core cause
> > behind the issue that this patch is to solve...
> 
> I've re-tried this, if you remove this patch and attempt to visit a
> non-exist sha1, *boom*
> 
> I can only speculate that CGI::Carp only redirects the output inside of
> perl, and does not handle the case when called programs (like git) write
> more directly to STDERR.

CGI::Carp doesn't redirect output: it adds timestamp and prints it to STDERR (unless one use 'carpout') to the result of 'die' and 'warn' calls.

*Without your series* when I visit non-existing sha1, or non-existing file I get correctly 404 error from gitweb. So you have borked something.

The CGI standard (http://tools.ietf.org/html/rfc3875) doesn't talk about 'standard error' stream at all; on the other hand it talks only about 'standard input' and 'standard output'. I have checked with simple CGI script in Perl, that neither using die or warn (both before any HTTP headers are send), neither with plain CGI or with mod_perl (ModPerl::Registry), with CGI::Carp I never get the error you see. Without CGI::Carp I get '500 Internal Server Error' instead of nicer one formatted by CGI::Carp, but I don't get it even without CGI::Carp with 'warn' and printing to STDERR directly.

The standard error stream either gets discarded (mod_cgid), or is written to /var/log/httpd/error_log (mod_perl).

-- 
Jakub Narebski
Poland