[cgiapp] CAD::Server strangeness

[George Hartzell]

Richard Jones writes:
 > George Hartzell wrote:
 > > Richard Jones writes:
 > >  > Using CGI::Application::Dispatch::Server, I'm getting a weird phenomenon 
 > >  > occurring:
 > > Can you provide a simplified test case that shows this behaviour, or
 > > share a copy of the app with me to walk into?
 > > 
 > > That output's being generated by CGI::Application::Dispatch, which is
 > > calling Data::Dumper:Dumper on the $args that are about to be passed
 > > to new().
 > > 
 > > It'd be interested to fire the server up under the perl debugger,
 > > break at that line, and see what's up with $args.
 > 
 > Hi George,
 > 
 > Well I've managed to narrow it down a bit - the problem was caused by 
 > defining the path/to/templates twice - once in TMPL_PATH => [ 
 > $path_to_templates ] in my Dispatch \%args_to_new hashref, and again in 
 > the webapp.cfg tt_config TEMPLATE_OPTIONS. As I can't avoid passing 
 > path/to/cfg in args_to_new, the fix is of course to omit TMPL_PATH.
 > 
 > I suspected this might be the case from the very simple demo app I put 
 > together for you (mailed separately), where I could see that the first 
 > request after server-start generated 2 '/path/to/templates' entries, the 
 > second request generated 4, third request generated 8, then 16, 32, etc. 
 > With my own app, the multiplication came much faster - the first request 
 > generated 8 '/path/to/templates' entries, second generated 64, and I 
 > didn't count the 3rd but would predict it was 512. There's probably a 
 > simple reason for this, but it's not that I've defined path/to/templates 
 > 8 times in my App!
 > 
 > However, the curious thing is - afaict, there is only one definition of 
 > path/to/templates in the demo app (and that's in the Dispatcher not the 
 > WebApp), so I've mailed it to you so you can investigate the phenomenon. 
 > Failure to include TMPL_PATH in the demos' Dispatcher is fatal as the 
 > WebApp can't find the templates, confirming that there is only a single 
 > source of templates path info. My guess is that the problem lies in the 
 > definition of TMPL_PATH in args_to_new() in the Dispatcher rather than 
 > in the webapps' config file. I'd also take a guess that it's something 
 > pretty simple to trace if you're handy with perl -d.

So....

There are at least a couple of problems conspiring here.

The simple one is in your demo app.  When one subclasses
CGI::Application::Dispatch and overrides dispatch_args, one needs to
make sure that it has the same semantics as what it's replacing.  In
particular, it needs to return a complete set of dispatch arguments
including a table entry.  One easy way is to just call
$self->SUPER::dispatch_args to get an initial hash ref then override
the fields you'd like to change.  It should probably also do something
intelligent with its own argument, which is hash of args that was
passed in to the call to dispatch().

The ugly problem hides the issue above and has to do with how
CGI::Application::Dispatch::Server does it's dispatching.  If you
specify your own CGI::Application::Dispatch subclass for ::Server to
use, then ::Server calls your class's dispatch_args method and passes
the result to CGI::Application::Dispatch->dispatch().  That dispatch
method goes to great pains to merge it's arguments into its own 
dispatch_args.  It starts off like this:

  sub dispatch {
    my ($self, %args) = @_;

    # merge dispatch_args() with %args with %args taking precedence
    my $dispatch_args = $self->dispatch_args(\%args);
    ...

The standard dispatch_args method looks like this:

  sub dispatch_args {
    my ($self, $args) = @_;
    return {
        default     => ($args->{default}     || ''),
        prefix      => ($args->{prefix}      || ''),
        args_to_new => ($args->{args_to_new} || {}),
        table       => [
            ':app'     => {},
            ':app/:rm' => {},
        ],
    };
  }

so after the first few lines of dispatch() we have a $dispatch_args
which has simply taken the values (if any) from %args for default,
prefix, and args_to_new.

Next dispatch() loops over the keys in $dispatch_args.  If there's a
key of "args_to_new" (which will either be empty or whatever was
passed in as an argument to dispatch() and stashed there earlier in
dispatch()) then it tries to do something intelligent about merging.
*** This is where the duplication is happening *** It pushes
$dispatch_args->{args_to_new}->{TMPL_PATH} on to
$args{args_to_new}->{TMPL_PATH}, and then ultimately patches the
resulting list into it's return value.  The problem is that since
$dispatch_args->{args_to_new} is a copy of $args{args_to_new} it's
just duplicated TMPL_PATH.

So, what should be changed:

  1) Richard should change his dispatch_args() method so that it
     matches the semantics of the one he's overriding.  In its current
     form we've seen that it's problematic with ::Server and it won't
     work at all under fastcgi using the fastcgi tricks discussed here
     recently.

  2) I think that "someone" (I think that Mark Stosberg holds the keys
     to CAD::Server at the moment, I'd be happy to take them back)
     should make some changes to ::Server.

        - change new() so that it stashes a copy of $p{class} into
          $self->{_class} and no longer saves
          $p{class}->dispatch_args. 

	- delete the entire ::Server::dispatch_args method.

 	- change handle_request() so that instead of 

          CGI::Application::Dispatch->dispatch(%{$self->{dispatch_args}})

	  it just does

	  $self->{_class}->dispatch();

	  and let's the CGI::Application::Dispatch {,sub}class do the
	  right thing.

	- add some tests to the package.  Any tests would be a start.

   3) I think that CGI::Application::Dispatch::dispatch is being too
      clever/helpful/broken in how it handles dispatch_args.  In
      particular, if someone calls dispatch with a hash that contains an
      args_to_new hash which in turn contains a TMPL_PATH array ref, then
      dispatch() ends up duplicating that array's values.

Number 2) above is the quickest fix to the problem.  Mark?

Number 1) will be necessary once number 2) is in place and is already
necessary if you want to run under fastcgi.

Number 3) seems to be a problem that no one has had yet but it's
lurking.

g.