xml-rpc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Rall <...@finemaltcoding.com>
Subject Re: PATCH: Refactor XmlRpcServer and friends for flexible threading.
Date Mon, 26 Aug 2002 18:01:48 GMT
"Andrew Evers" <aevers@redwood.nl> writes:

...
> >> c. The HandlerMapping interface is a single method
> >>      Object getHandler(String methodName)
> >>    this will return null for no mapping.
> >
> > Why not use the Map interface instead of a custom HandlerMapping?
> > Though HandlerMapping might have a narrower interface (which is nice),
> > is it really worth the trade-off of introducing a new object type? The
> > less custom object types involved in a package, the easier I find it to
> > understand (especially as a new developer/user).  It seems that
> > alternate mapping types could be just as easily supplied through
> > custom Map implementations.
> 
> Two reasons (other than the narrower interface):
> 1. The current patch specifies that the HandlerMapping will throw an
>    Exception if the handler is not found. This allows better error
>    propagation than simply returning null.
...

Is a handler name which isn't resolvable really an error to a
XmlRpcHandlerMapping implementation?  To the caller of getHandler(),
sure it is.  But is it to the XmlRpcHandlerMapping itself?  (I'm not
sure.)  I prefer to use exceptions only in exceptional situations (see
"The Practive of Programming").

> I think calling through an XmlRpcHandlerMapping is clearer than
> through a generic Map, but that could just be me.

IMHO, a properly named and documented argument removes any clarity
issues.  This is moot since we'd have to use Hashtable, which is a
concretion instead of an interface.  Bleh.

...
> New classes effectively already described:
> + XmlRpcHandlerMapping - the getHandler(String) throws Exception interface.
> + DefaultHandlerMapping - an implementation of HandlerMapping based
>   on the existing XmlRpcServer implementation.
> + ParseFailed - A runtime exception a'la AuthenticationFailed.
> + Invoker - the Invoker class previously in XmlRpcServer.java, now public.
> + XmlRpcRequest - encapsulates an XML-RPC request.
> 
> Completely new classes:
> + XmlRpcRequestProcessor - decode a request
>   Produce an XmlRpcRequest from an InputStream (optional user/pass).
>     public XmlRpcRequest processRequest(InputStream, String, String)
>     public XmlRpcRequest processRequest(InputStream)
> + XmlRpcResponseProcessor - encode a response/exception
>   Produce a byte [] from either an Object - representing a return value
>   or an Exception - representing an error.
>     public byte [] processException(Exception x, String encoding)
>     public byte [] processResponse(Object outParam, String encoding)
> + XmlRpcWorker - decode, process and encode a request/response.
>   Ties everything together, but only communicates with the XmlRpcServer
>   via XmlRpcServer -> XmlRpcWorker execute() and XmlRpcWorker calls via
>   the XmlRpcHandlerMapping.

Can anyone think of a more descriptive name for this than "worker"?

> Current classes with new roles:
> + XmlRpcServer - handle a thread pool and a default handler mapping.

You should probably note this in the JavaDoc.

> Things changed a little from the patch I originally proposed. The
> whole thing is probably best explained by four lines in XmlRpcWorker:
> 
>   request = requestProcessor.processRequest(is, user, password);
>   handler = handlerMapping.getHandler(request.getMethodName());
>   response = invokeHandler(handler, request);
>   return responseProcessor.processResponse(response,
>   requestProcessor.getEncoding());
> The *Processor classes are public and have public entry points so that
> it is possible to build your own XmlRpcWorker-like classes, by assembly
> (ie. delegation) rather than by inheritance, which can be trickier.
> 
> I've copied in licenses, copyright dates and redistributed authors
> as best I could, please don't be offended if I've made a mistake here ;)
> 
> Otherwise, let me know what you think.

It's a beautiful patch, great job.  The test cases even still run (I'm
probably imagining it, but they seemed to run a tad slower than
before).  I've committed your changes to CVS HEAD with very minor
mods.  Please check out a fresh copy and shout if I've missed
anything.
-- 

Daniel Rall <dlr@finemaltcoding.com>

Mime
View raw message