On Sun, Sep 11, 2011 at 11:25:20PM +0100, Andrew Ford wrote:
> I suggest that we move this discussion to dbi-dev.
[dbi-users dropped]
> On 11/09/11 18:14, Tim Bunce wrote:
> >On Fri, Sep 09, 2011 at 03:50:53PM +0100, Andrew Ford wrote:
>
> my $next_middleware = $go_transport->middleware;
> $go_transport->middleware( sub { return $middleware_class->handler(shift,
> $next_middleware) } );
>
> [...]
> >>Each middleware handler would take a request object and a reference
> >>to the next handler in the chain.
> >
> >Calling $go_transport->middleware within the sub isn't right if we go
> >with a chain of closures, which I'd prefer. I'd expect that last line
> >to be something like:
> >
> > $go_transport->middleware( $middleware_class->new_handler(
> > $go_transport->middleware ) );
> >
> >where new_handler would look something like:
> >
> > sub new_handler {
> > my ($class, $existing_handler) = @_;
> >
> > my $new_handler = sub {
> > my ($self, $request) = @_;
> > ...do something with $request...
> > my $response = $existing_handler->($self, $request);
> > ...do something with $response...
> > return $response;
> > } );
> >
> > return $new_handler;
> > }
> >
> >So at setup-time a chain of closures is created.
>
> [...] Your approach
> is probably cleaner, but it does put the onus on middleware writers
> to build the closures properly (although that can be mitigated
> against with good documentation that provides example code for
> writing new_hander).
It's pretty trivial really.
> dbi:Gofer:transport=null;middleware=delay(5),module2(a=1,b=2);dsn=dbi:XXX...
I'd rather not require the middlewares to do argument parsing.
It seems inapprpriate. The transport base class can do the typical
incantation of:
%args = map { split(/=/, $_, 2) } for split(/,/, $args_string);
which for '5' would yield %args = (5,undef). The middleware could detect
that kind of case (single arg with an undef value) and deal with if it
really wanted to.
> There is still the issue that the middleware gets invoked before the
> transport... sort of - actually it is the transport that is
> invoking the chain of closures that make up the middleware.
The transport is split into two parts, one handles the caching, retry
and timeout logic (in the base class) and the other does the "get this
request from A to B" logic (in the various sub classes).
The middleware wraps the latter part, and so gets the caching, retry and
timeout logic for free.
> Anyway,
> linguistically with the DSN I am saying that "I want the null
> transport to be used... and by the way I want it to pass the request
> through these items of middleware" - so I think that outermost-first
> would be what I at least would expect.
If we go that way (I'm undecided) then the examples should probably
have the via=... before the transport=...
> Of course one could model it as filters, rather than layers, and have
> pre=... and post=... components but that would be more tricky to implement
I think YAGNI applies.
> >Other server-specific stuff in DBI::Gofer::Execute might need to move out
> >(perhaps to a DBI::Gofer::ServerExecute subclass) so it can be used as a
> >base class for middlewares that want to appear to 'execute' some requests.
s/as a base class for/by/;
> >Certainly middlewares ought to inherit from a base class supplied by the
> >DBI to enable safer evolution.
> Should the middleware inherit from DBD::GoferMiddleware::Base or
> DBD::Gofer::Middleware::Base
DBD::Gofer::Middleware::Base. Though the class would be practically empty.
> And what class should we use as the default class prefix? Are we
> still saying that unqualified middleware classes are assumed to have a
> prefix of DBDx::GoferMiddleware?
Yes, I think so.
> I would like to have some methods that can be called without having
> to understand the layout of the request and response objects.
Yeap. They need better encapsulation and convenience methods. But let's
leave that to one side for now and revisit it once there's working code.
> is coming up with some new unit tests that thoroughly exercise the
> new code, in combination with other transports as well as the new
> "passthrough" transport.
> >>(are we keeping the final class name component for the middleware
> >>class in lower case, as it is with the transport classes?).
> >I think I'd prefer a capitalized name for these.
>
> How does one cope with capitalized class names and lower case
> component names - or are you saying that we suggest that middleware
> writers use capitalized class names and thus the names listed with
> "via=" would be in upper case (to distinguish them from transport
> names?)?
Perhaps a better scheme is that DBI supplied things (transports and
middleware) are lowercase but externally supplied things are capitalized.
> >>If we have a DBD::Gofer middleware feature then it would be helpful to
> >>add a new passthrough (or "passthru") transport. This would be based
> >>on the null transport, but without the serialization/deserialization,
> >>i.e. something as simple as:
> >For efficiency reasons the DBI::Gofer::Execute class assumes it can
> >modify elements of the request object in-place. The serialization is
> >needed to avoid subtle bugs that that would otherwise cause. I looked
> >into 'fixing' that some years ago but there wasn't a good use case then
> >- the null transport was only ever used for testing. It's probably not
> >worth trying to fix though. The overhead of serialization/deserialization
> >is probably (I'm guessing) a relatively small part of the overall
> >overhead of using Gofer.
> >
> >Feel free to try, but I wouldn't make it a priority :)
>
> I noticed those "subtle bugs" when I added the "passthrough"
> transport to the DBI tests (85gofer.t). I fixed them by using
> Storable::dclone() on the request; the response does not need to be
> serialized/deserialized.
I'd be happy for that transport to be called 'nul' (one 'l').
It functions identically to the 'null' transport but it's faster.
It's also faster to type ;-)
> I am also wondering whether this transport
> should be connection-oriented, as it is going to be used to proxy to
> other local DSNs, so I can't see the point in re-connecting for each
> request.
Gofer doesn't support connection-oriented transports in the sense that
I think you mean here. The ssh transport is connection-oriented in the
sense that it holds an ssh connection open for multiple requests, but
each request is processed in a connectionless way.
The DBI::Gofer::Execute code (_connect) defaults to connect_cached, so
it doesn't re-connect for each request.
Having said that, I'd *love* to see connection-oriented *sessions*
because that would be a key enabler for the one big feature that Gofer
still lacks: transactions.
But let's not go there just yet. There's lots to be done here first.
It would make a great follow-on project for you if you're interested!
Tim.
