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

[Jakub Narebski <jnareb@gmail.com>]

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

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