PATCH: Refactor XmlRpcServer and friends for flexible threading.

[Andrew Evers <ae...@redwood.nl>]

> "Andrew Evers" <ae...@redwood.nl> writes:
>
> Looks right on, and worth adding to its header documentation.  I'd do
> it myself, but don't want to interfere with your changes; would you
> include this list of responsibilities in your patch?

Shall we wait for the discussion to die down? ;)

>>      XmlRpcRequest parseRequest(InputStream, String, String)
>>      XmlRpcRequest parseRequest(InputStream)
>
> Excellent suggestion!  This follows the pattern set by the servlet API,
> Apache's httpd, and many other request/response app framework I've
> worked with.  Were you thinking protected or public for the
> method scoping?
>
> I would be +1 on deprecating the XmlRpc class, moving its content into
> a new XmlRpcProcessor class, and having the original XmlRpc class
> extend the new one.
>
>> b. XmlRpcProcessor also provides an executeRequest method that
>>    will use a callback interface to map handlers to objects.
>>      void executeRequest(InputStream, String, String, HandlerMapping)
>>      void executeRequest(InputStream, HandlerMapping)
>
> I like it.  Why do to suggest passing the HandlerMapping into the
> method call rather than having the XmlRpcProcessor maintain that
> information internally?
>
>> 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.
2. It works with Java 1.1.

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

>> 3. The changes above allow external management of threads
>
> Yay!
>
>> 4. Splitting out the Invoker has recently been suggested by Kevin
>>    Hester (my formatting):
> Yup.

OK, the patch is attached in "cvs diff -u" format, with extra files
attached independently. Everything is in a JAR file, since I hope
you all have jar ;) Commentary for the patch follows.

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.

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

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.

Andrew.

Re: PATCH: Refactor XmlRpcServer and friends for flexible threading.

[Andrew Evers <ae...@redwood.nl>]

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

Well, I tend to think of a missing handler in an operational XML-RPC
system as calling a method that does not exist. In a production
machine-machine environment this is very much the case. Theexception also adds value by allowing the mapping implementation
to specify the error message, so that it is possible to say
'too busy', 'backend down' or 'no such method'. Returning null
is not as expressive.

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

Not I ;)

>> Current classes with new roles:
>> + XmlRpcServer - handle a thread pool and a default handler mapping.
>
> You should probably note this in the JavaDoc.

I have a few more modifications coming through that externalize
the XmlRpc.Value class (to allow for a cleaner method to customize
the type mapping). The JavaDoc patches will come with this, followed
by some test cases in JUnit.

> ... Please check out a fresh copy and shout if I've missed
> anything.

You've missed Invoker.java ;)

Andrew.

Re: PATCH: Refactor XmlRpcServer and friends for flexible threading.

[Andrew Evers <ae...@redwood.nl>]

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

Well, I tend to think of a missing handler in an operational XML-RPC
system as calling a method that does not exist. In a production
machine-machine environment this is very much the case. Theexception also adds value by allowing the mapping implementation
to specify the error message, so that it is possible to say
'too busy', 'backend down' or 'no such method'. Returning null
is not as expressive.

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

Not I ;)

>> Current classes with new roles:
>> + XmlRpcServer - handle a thread pool and a default handler mapping.
>
> You should probably note this in the JavaDoc.

I have a few more modifications coming through that externalize
the XmlRpc.Value class (to allow for a cleaner method to customize
the type mapping). The JavaDoc patches will come with this, followed
by some test cases in JUnit.

> ... Please check out a fresh copy and shout if I've missed
> anything.

You've missed Invoker.java ;)

Andrew.

Re: PATCH: Refactor XmlRpcServer and friends for flexible threading.

[Daniel Rall <dl...@finemaltcoding.com>]

"Andrew Evers" <ae...@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 <dl...@finemaltcoding.com>

Re: PATCH: Refactor XmlRpcServer and friends for flexible threading.

[Daniel Rall <dl...@finemaltcoding.com>]

"Andrew Evers" <ae...@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 <dl...@finemaltcoding.com>