Subject: Re: [PATCH 0/2] gitweb: Add support for running gitweb as FastCGI script

From: Jakub Narebski <[email-masked]>
Date: Fri, 14 May 2010 12:53:42 +0200

On Thu, 13 May 2010, Peter Vereshagin wrote:
> 2010/05/11 15:51:15 +0200 Jakub Narebski <[email-masked]> => To Peter Vereshagin :
>> On Tue, 11 May 2010, Peter Vereshagin wrote:
>>> 2010/05/11 12:58:50 +0200 Jakub Narebski <[email-masked]> => To Peter Vereshagin :
 
>>>>>> I have changed this 'exit' to non-local goto to toplevel.  It could be
>>>>>> done instead by redefining 'exit' subroutine, like shown below, but I
>>>>>> feel that would be hacky if you can change gitweb code (it is not
>>>>>> black box you should not touch).
>>>>> 
>>>>> Right, one shouldn't ever redefine perl built-in functions. I did only because
>>>>> of no other way to 'get things working'
>>>> 
>>>> Why not?  For example CGI::Carp redefines 'die' to log errors.
>>> 
>>> Ouch, sorry, I meant 'last' or something like that.
>> 
>> "last" / "last LABEL" is a command, not a function, therefore you cannot
>> redefine it.
> 
> It's a flow control statement, thus it is a built-in thing; same way as any other
> functions are explained in a 'perldoc -f'.

`perldoc -f exit` says 'The exit() function ...', while `perldoc -f last`
says 'The "last" command is like the "break" statement in C ...'.

> Therefore it is treated by monkeys crowd as function. It's obvious for me to
> stay out here (here != maillist) yet in such an environment.

Sidenote: The 'Monkey patch' article on Wikipedia says that the
technique of adding method dirctly to class instead of subclassing was
originally called "guerilla patching", then it mutated into "gorilla
patching", and finally into "monkey patching".

> Anyway, I compare "last" here  with exit() and die() which look to user just
> like the same kind of: the flow control statements. I guess any Perl user who
> makes things like gitweb (at least as a CGI-only app) shouldn't care about
> such an internal difference of flow control statements those are
> hidden/incapsulated inside the implementation of those statements?

Perl hacker should know the difference between command such as "last"
and "next", and functions such as exit() and die().  Just like C
programmer should know the difference between "break" statement and
exit() function.

> Needless to mention that the 'last LABEL' ( goto, gosub, ... named them )  is a
> bad and a very deprecated style which is every schoolboy is aware about
> nowadays to keep from using in the application [...]

Not true.  The 'last LABEL;' command is very useful to exit nested
loops.  If used right it makes code much simpler (allowing to avoid
extra flag variable and/or complicating loop conditional).  If I
remember correctly in O.-J. Dahl, Edsger W. Dijkstra, C. A. R. Hoare
"Structured Programming" the programming language described includes
"break <n>" statement, with similar purpose as "last LABEL" in Perl.

Note also that Dijkstra wrote in seminal article "Go To Statement
Considered Harmful" that the problem with abused 'goto' is that it
compilcates and muddles control flow of program.  But there are
legitimate uses of 'goto' that make the program simpler to understand,
and not harder,... among those is handling exceptions.

>>>> I know this from painful experience of trying to find bug in a
>>>> test... when the error was in parsing file in 'do $file;'.
>>> 
>>> I handle them just fine like in any other CGI program using
>>> CGI::Carp:fatalsToBrowser. Are you about to 'make test' via the http? ;-)
>> 
>> I don't think you understand what I wanted to say there.
>> 
>> If you don't check if there were parse errors from 'do $file;', you can
>> get later some error message which is totally unrelated to the parsing
>> error.  If you don't know or forget that you should check $@ after 
>> 'do $file;', and are unlucky, you can chase elusive error from there
>> to kingdom come...
> 
> Got it, it's about the inclusion failure via the do() which is the
> development, not a production, situation.

Yes, it is a problem mainly in developemtn, where changes to the file
included via "do <file>" might introduce parsing errors.

> I think this should be an adjective noun to use the both strict and
> the warnings?

The problem is that "do <file>;" is similar to "eval `cat <file>`;"
(except that it's more efficient and concise), it that it silences
parsing errors.  From `perldoc -f do`:

  If "do" cannot read the file, it returns undef and sets $! to the error.
  If "do" can read the file but cannot compile it, it returns undef and sets
  an error message in $@.   If the file is successfully compiled, "do"
  returns the value of the last expression evaluated.

> And yes, since it's about development but not production use, die is just fine
> in the inclusion code like this:
> 
> eval( 'use Module;' ); die $@ if $@;

Wrong!
 
> as always, require() can do the trick, not to mention usual 
> 
> use Module;
> 
> This all will cause die() when it's necessary as only the application developer
> knows how strict is the dependence on the Module. In some cases, application
> can work without some Module but it's just better with it.

First, both "use Module;" and "require Module;" (and "require '<file>';")
do automatic error checking and raise an exception if there is problem.

Second, "use Module <LIST>;" is equivalent to

  BEGIN { require Module; import Module <LIST>; }

and therefore it doesn't make sense to use it for conditional inclusion.

Therefore, to load Perl module / file, if you can 'die' you can simply
use

  require "<file>";

If you don't want to die, but want to know if loading and parsing file
succeeded or not, you should use the following syntax:

  if (eval { require "<file>"; 1 }) {
    ...
  } else {
    ...
  }

If you want to use 'do "<file>";' (it is preferred in some
circumstances), you really should check for error conditins:

  unless (my $return = do "<file>") {
    if ($@) {
       # couldn't parse <file>
    } elsif (!defined $return) {
       # couldn't do <file> (e.g. couldn't find <file>)
    }
    ...
  }

[...]
>> PSGI is interface, Plack is reference implementation.  You can run PSGI
>> app on any supported web server; this includes running PSGI apps on
>> FastCGI.
> 
> Existing problem FCGI::Spawn for is not the PSGI applications to be run as a
> FastCGI, but the bunch of existing CGI.pm applications (even gitorious) need
> to be more effective with the widest-spread protocol FastCGI. Best without any
> patching of the application, deployed the same simple way as with apache's cgi
> implementation.

Gitorious is in Ruby, therefore is not a CGI.pm application, as it is
not even in Perl.

By using Plack::App::CGIBin you can load CGI scripts from a directory
and convert them into a <persistent> PSGI application.  You can use
Plack::App::WrapCGI to convert single CGI script into PSGI application.
You can use Plack::Buuilder's domain specific language to join (map)
together a bunch of PSGI applications (in different paths) in a single
app (via Plack::App::URLMap).

You can then run PSGI application (for example the PSGI app which loads
CGI apps via Plack::App::CGIBin) on any supported web server, which
includes FCGI (FastCGI).

-- 
Jakub Narebski
Poland