OK, I now have a patch that I would like to officially submit for
review and for inclusion in GWT 1.4.

This is for issue 389:
http://code.google.com/p/google-web-toolkit/issues/detail?q=389&can=2...
As discussed in this earlier thread:
http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse...

The main changes in the patch, compared to the first draft, are:

- Changed RPCCall.invoke() to throw InvocationTargetException when
unexpected exceptions are thrown by the callee, as per ScottB's
suggestion in the earlier thread.

- Renamed DelegatingServiceServlet to BaseServiceServlet.

- Exposed BaseServiceServlet utility methods as public static methods.
(George Georgovassilis, please take a look -- I think you can write
your Spring GWTHandler with very few lines of code, by reusing these
methods.)

- Inverted the structure of BaseServiceServlet / RemoteServiceServlet.
Instead of having BaseServiceServlet define an abstract
"fetchServiceDelegate" method, BaseServiceServlet now defines an
abstract "handleRPCCall" method.  The implementing subclass
(RemoteServiceServlet or otherwise) now gets an RPCCall to dispatch
itself, rather than fetching a service object which BaseServiceServlet
invokes.

This turned out to be a better design when I implemented the more
flexible RPC in the g4jsf project.  g4jsf uses JSF event routing to
invoke GWT RPC on a JSF component.  It turned out to be very clean to
pass the GWT RPCCall object in the g4jsf event; basically, the RPCCall
became the envelope for passing the GWT RPC call to the JSF server-side
component.  Passing down the reified RPCCall means it can be routed
however the server wants.  (The earlier design would have forced the
top-level service to retrieve the component itself, which was much more
awkward.)

I have more I plan to do with this, and I'm considering writing some
additional tests (there seem to be no tests of expected vs. unexpected
exception handling with RemoteServiceServlet), but the patch is
definitely ready for review and hopefully even for submission.  Please
review; I welcome, and will act on, all comments.  The patch passes
"ant test" in trunk.

Thanks very much :-)
Cheers!
Rob

Nice :)

I'll look shortly into it. Thank you alot!

Hello Rob,

I'm taking a look at this patch as well.  I'll have my feedback for you
shortly.

Cheers,

On 1/23/07, Rob Jellinghaus <r...@unrealities.com> wrote:

--
Miguel

Thank you, George and Miguel!

Reviewing my own patch, I missed deleting an obsolete comment at lines
279 to 282 of BaseServiceServlet.java.  Also, the third sentence of the
header comment to BaseServiceServlet.processCall should be moved to the
header comment of RemoteServiceServlet.handleCall.  I'll wait for
actual feedback from you guys before doing anything, though.

Cheers!
Rob

I implemented the GWTHandler on top of your changes and it works fine.
You know already that I am overly fond of your changes, so I'll just
stick to the criticism :)

1. At first I found myself copy & pasting the access checks from the
RemoteServiceServlet and it occurred to me that I could reuse that code
- with a very small change: don't check 'this' and 'getClass()' for
interfaces but a provided object 'service' (which the
RemoteServiceServlet can invoke of course with 'this'). This of course
does not allow anymore for the 'knownImplementedInterfaces', but one
could provide that as a further argument perhaps?

2. Since the ServletContext is used only for logging purposes, maybe
you can remove it from writeResponse and respondWithFailure ? A Spring
managed service will typically use its own logging and I think other
use cases may arrive from that.

3. Exception handling. This is probably something we should discuss in
general about RPC server side tasks. I am not very fond of the idea to
serialise back to the client anything else than an explicit
SerializableException because, in general, this would break
transactions around services that are merged with RPC just like the
standard practice of extending the RemoteServiceServlet. Spring for
example wraps services with proxies which open & close JTA transactions
around method invocations. The convention here is that unchecked ( =
not declared in the method signature, RuntimeException) exceptions
cause a rollback of the transaction while checked exceptions don't. If
one now would wrap the transaction management outside of the RPC layer,
SerializableExceptions would not cause a rollback (that's good) but
other catched and thrown exceptions as well - and that is not so good.

Good work! And I'm glad you did it :)

That's my wishlist for now.

Yes, I considered that, but I was thinking it is nearing the point of
diminishing returns.  There could be BaseServiceServlet (abstract
handleCall()) *and* DelegatingServiceServlet (abstract
fetchServiceDelegate()) *and* RemoteServiceServlet, but it just felt
like not enough to matter.  I tried to factor out as many of the
helpers as possible into the RPC.java class -- maybe a bit more could
be done there?

Good point, I'll look into that.

But only checked exceptions that are declared in the method signature
get serialized back to the client.  See RPCCall.invoke().  So unchecked
exceptions never get serialized back to the client, and neither do
unexpected checked exceptions.

However, you make a good point -- unchecked exceptions arguably
shouldn't be wrapped in an InvocationTargetException at all, but right
now, they are.  That will force the framework to unwrap them and
rethrow them, which seems wrong.  It looks like Method.invoke() will
wrap all invokee exceptions (including unchecked ones) in
InvocationTargetException.  RPCCall.invoke() could instead unwrap them
and let them escape.  Thoughts?

Thanks, I'm glad it looks good to you!  I will soon finish up
integrating general interfaces into g4jsf and then submit that once GWT
1.4rc1 (or some other prerelease build) ships.  Then it's on to
integrating GWT with Seam!

Cheers,
Rob

Whoops, didn't mean to be presumptuous in my last paragraph... I'll do
all that IF the GWT devs approve my patch ;-)  (which I don't take for
granted!)

Hey Rob,

Sorry that it has taken me longer to get this out.  I think that you have
all of the right pieces.  My comments are meant to explore whether a further
refactoring might result in the same flexibility but with less exposure of
the details.  For example, it seems that things like RPC.isExpectedException,
RPC.getClassFromName, or RPC.findInterfaceMethod may not really need to be
exposed.  As a result, I would consider changing the API of the RPC class to
the following one (all non-essential details have been left out):

/**
 * Utility class that encapsulates the RPC decoding, encoding functionality.
 */
public final class RPC {
    [Adding this member will remove the need to create one of these for
every RPC
    request that comes in.]
    private static final ServerSerializableTypeOracle serializableTypeOracle
= ...;

    /**
     * Returns a <code>RPCRequest</code> instance that is built by
     * decoding the contents of the RPC payload.
     *
     * @param payload the RPC payload encoded as a UTF-8 string
     * @return RPCRequest instance which contains all of the RPC request
     *         parameters
     * @throws SerializationException
     */
    public static RPCRequest decodeRequest(String payload)
        throws SerializationException { ... }

    [The following method is not necessary but it would allow a developer to
implement their own invocation logic yet still use our standard encoding.
What do you think?]
    /**
     * Encodes the result of an RPC method invocation into a string.  This
includes
     * dealing with how exceptions are returned to the caller.
     *
     * @param serviceMethod method whose response we want to encode
     * @param result object that is the result of invoking the RPC method.
This
     *          maybe a regular object or it could be an exception.
     * @param isException true if the object was thrown by the call
     * @return service method result encoded as a string
     */
    public static String encodeResponse(Method serviceMethod, Object result,
        boolean isException) throws SerializationException { ... }

    /**
     * Returns a string that encodes the result of invoking the service
method
     * with the specified parameters.
     * Note: that this method calls
     * {@link com.google.gwt.user.server.rpc.RPC.RPC_#encodeResponse(Method,
Object, boolean) encodeResponse(Method, Object, boolean)} internally.
     *
     * @param serviceMethod method to invoke
     * @param object object on which to invoke the method
     * @param params parameters to pass to the service method
     * @return string encoding the result of the method invocation
     * @throws Throwable thrown if the exception is not a serializable
     *         runtime exception or a checked exception that the service
     *         method is specified to throw
     */
    public static String invoke(Method serviceMethod, Object object,
        Object[] params) throws Throwable { ... }

Currently, your RPCCall class does most of the work.  However, if you
refactor the RPC class, you could really thin out the RPCCall class.  For
that matter the RPCCall class could simply be renamed the RPCRequest class
since it would simply encapsulate the RPC request information.  Something
like:

     /**
      * Encapsulates the components of an RPC request.
      */
public class RPCRequest {
     public RPCRequest(Class serviceInterface,
         Class[] parameterTypes, Object[] parameterValues) { ... }
     public Class getInterface() { ... }
     public Method getMethod() { ... }
     public Class[] getParameterTypes() { ... }
     public Object[] getParameterValues() { ... }

The changes would also relax the need for adding the BaseServiceServlet to
the servlet class hierarchy.  The reason is that you can override the
RemoteServiceSevlet.processCall(String) method today.  So if you wanted to
create a delegating service servlet, you could simply subclass
RemoteServiceServlet, override the processCall(String) method and then use
the RPC utility class as is appropriate for your solution.  For that, matter
you could even use the refactoring outside of a servlet environment.

What do you think?

On 1/23/07, Rob Jellinghaus <r...@unrealities.com> wrote:

--
Miguel

No worries, one day turnaround is great :-)

Your proposal (let me know if I paraphrase accurately) is to split the
decoding out of RPCCall, leaving just the "envelope" state of the
method invocation, and renaming it RPCRequest.  Then the RPC class has
all the behavioral utility methods -- one for decoding the request, and
one for encoding the response -- and the RPCRequest class is a simple
bean.

That works for me, with the caveat that it loses one of the
optimizations / security properties of the original
RemoteServiceServlet implementation.  That implementation decodes the
incoming request payload progressively, checking each piece of the
envelope (interface name, method name, method argument types) as it
decodes.  If it finds an invalid part of the envelope, it terminates
immediately with a SecurityException.  So if a request with an invalid
interface or invalid method comes in, the servlet never wastes time
decoding the entire request -- which might be a mild barrier against
(clumsy) denial of service attacks.

The RPCCall implementation in my last patch, by bundling the
deserialization into the envelope object, preserves this behavior -- if
all you ask for is the interface name, that is all that gets decoded
from the stream, and so on.  So the RemoteServiceServlet.handleCall
method effectively has the same deserialize-while-checking behavior as
the original implementation.  If that behavior is not necessary, then
having a single RPC.decodeRequest method -- that always deserializes
the entire request -- is definitely simpler.

If we do decide to keep the deserialization behavior coupled to the
envelope object, as in the current RPCCall, we could and should still
hide the RPC utility methods you suggested hiding.  (In fact, we could
hide the RPC class altogether inside server.rpc.impl if we wanted....)
I exposed them originally as mild helpers to developers implementing
their own service method invocation, but any developer who's doing that
probably already understands java.lang.reflect fairly well, and the API
clutter likely isn't worth it.

I'm not sure how much utility there is in this, given that the
developer has to understand and properly implement the exception policy
(that expected exceptions are encoded back to the caller).  In my g4jsf
work, there's no need for this particular method, and it sounds like
George G is also OK without it.  So I'd say this could be omitted.

I don't like throwing Throwable here; it's too vague.  I am not
positive what a better policy would be, but here's a stab at one:

@throws RuntimeException if the service method throws a
RuntimeException (this is not declared, of course)
@throws SerializableException if the service response cannot be
serialized
@throws InvocationTargetException if the service method throws an
unexpected checked exception (one not declared in the method
signature); the unexpected exception will be the cause of the
InvocationTargetException

This seems clearer to me, and provides a tighter API for the developer
to build catch clauses around, while still preserving the necessary
flexibility.  (I now think this would be the right signature for
RPCCall.invoke() if we keep it.)

This is a good point.  I've become accustomed to the (possibly flawed)
Design for Inheritance pattern, whicih tends to force subclasses.  But
you're right, overriding is very easy.  For that matter, we could
collapse BaseServiceServlet back into RemoteServiceServlet, but keep
the RemoteServiceServlet.handleCall(RPCRequest) method, renaming it to
RemoteServiceServlet.processRequest(RPCRequest).  Then developers could
override either RemoteServiceServlet.processCall(String) OR
RemoteServiceServlet.processRequest(RPCRequest)., whichever is more
convenient for them.

It all makes a lot of sense.  Your turn :-)
Thanks very much for the careful review!
Cheers!
Rob

Hi Rob,

My comments are interleaved below.  Note, that I included a summary of the
changes based Rob's feedback at the end of this email.

On 1/24/07, Rob Jellinghaus <r...@unrealities.com> wrote:

Yes.  The RPCRequest class simply encapsulates the request parameters.

That works for me, with the caveat that it loses one of the

Whenever possible, I prefer simplicity.  There is also some appeal to me in
knowing that once you have an RPCRequest instance, you won't get any other
errors while querying its properties.  If it was constructed then it is good
to go.

Where my proposed change could run into an issue is the case where, an RPC
request is received for a valid method on a valid service interface that can
be found on the server's class path that is not implemented by the instance
that you will eventually call the method on.  In that event we would decode
the payload only to find out that we did not need to.  If the interface is
not available on the class path or if it is not a valid service interface or
if the requested method is not valid, then decodeRequest would early out and
do no further decoding.

Also, a user who used the RPC/RPCCall system would also have the same
problem if they did not call a method like
RemoteServiceServlet.isImplementedRemoteServiceInterface(String) first.
I'm glad that you brought it up.  Because, it made me think of the following
possibilities.

There are two possible avenues that we could take here.  The first is to
give the RPC.decodeRequest the option of constraining its decoding against a
particular Class instance.  If that class instance does not implement the
service interface then the implementation would error, early out, and
therefore not decode anymore than is necessary.  So, we could add an
overload for the decodeRequest method (or introduce a new name) that
actually constrains the decoding of the request against a particular Class.
This may, in fact, be better because then we actually move
RemoteServiceServlet.isImplementedRemoteServiceInterface into the RPC class
as a private member and anyone who called the constrained version of
decodeRequest would use it implictly.  It would look something like this:

    /**
     * Returns a <code>RPCRequestParameters</code> class that is built by
     * decoding the contents of the RPC payload.
     *
     * NOTE: this method call decodeRequest(String, Class) with null for the
     *       class parameter.
     *
     * @param payload the RPC payload encoded as a UTF-8 string
     * @return RPCRequest instance which contains all of the RPC request
     *         parameters
     * @throws SerializationException
     */
public static RPCRequest decodeRequest(String payload) throws
SerializationException { ... }

    /**
     * Returns a <code>RPCRequestParameters</code> class that is built by
     * decoding the contents of the RPC payload against a class that
constrains
     * the set of valid service interfaces.
     *
     * @param payload the RPC payload encoded as a UTF-8 string
     * @param constraintClass if non-null the implementation checks that
this class
     *        actually implements the service interface requested in the
payload
     * @return RPCRequest instance which contains all of the RPC request
     *         parameters
     * @throws SerializationException
     */
public static RPCRequest decodeRequest(String payload, Class
constraintClass) throws SerializationException { ... }

If that sounds palatable then we could simplify RPCRequest to only contain
the service method and the request parameter values.

The second solution would be to change the RPCRequest class constructor
signature and to delay load the values which would force the access to throw
checked exceptions.

If we do decide to keep the deserialization behavior coupled to the

Definitely, some of the stuff that is currently public to RPC needs to be
hidden.

Actually, the more I think about this the more I think that having the
invoke method is not necessary at all.  This is a little more aggressive but
bear with me for a moment.

Today, invoke is a fairly trivial wrapper around the
java.lang.reflect.Method.invoke.  The real value in our implementation of
invoke comes from deciding what to pass to the createResponse method, in the
try or catch blocks.  So, it seems like the encodeResponse method could be
retained and we could actually do away with the invoke method.  The code
could look something like this:

public String encodeResponse(Method serviceMethod, Object result, boolean
isException) throws SerializationException {
  if (isException) {
    if (!isExpectedException(serviceMethod, result.getClass())) {
      throw new SerializationException(...);
    }
  }

  return createResponse(new ServerSerializationStreamWriter(...), result);

The net effect is that the core of BaseServiceServlet.processCall could be
writen as follows:

String processCall(String payload, HttpServletRequest request,
HttpServletResponse) throws SerializableException, IllegalAccessException {
    ...
    RPCRequest rpcRequest = RPC.decodeRequest(payload, getClass());

    Method serviceMethod = rpcRequest.getMethod();
    Object result;
    boolean isException = false;
    try {
      result = serviceMethod.invoke(this, rpcRequest.getParameterValues());
    } catch (InvocationTargetException e1) {
      result = e1.getCause();
    }
    return RPC.encodeResponse(serviceMethod, result, isException);
    ...

Thanks for calling me out on the Throwable.  That was a bad idea.  If you
buy my argument for doing away with invoke then the issue of what exact type
of exception to throw goes away.
...

read more »

Son of a... gun.  I wrote an entire response to this in the browser,
clicked Post, Google Groups said "Your post was successful", and then
it vanished into the bit bucket :-(  Three hours later, no sign of it,
and other posts have arrived.  And I couldn't for some reason use the
back button to get back to my post, or maybe I fat-fingered it.  SIGH!

I hope it doesn't show up 2 minutes after I post this.  If it does, my
apologies.  Oh well, rewriting something is always quicker the second
time.

About whether to deserialize inside RPCRequest:

Makes sense, and it looks like we can even preserve the progressive
decoding given your constraintClass proposal below.

Brilliant, makes perfect sense.  One issue, though: currently,doing the
implementation check in RemoteServiceServlet enables each
RemoteServiceServlet instance to keep a private interface lookup cache.
 How important is that optimization?  Would we want to have a static
synchronized interface cache in the RPC class, to deliver that same
optimization?  Or can we just trust modern JDKs to be reasonably
efficient at the lookup?  (The RPC class would also have to implement
the same superclass interface lookup algorithm as the current
RemoteServiceServlet, which might be another reason to keep the caching
behavior....)

Looks good, except this commingles genuine SerializationExceptions with
exceptions caused by the interface or method not being defined.  I
would prefer:

/**
 * Returns a <code>RPCRequestParameters</code> class that is built by
 * decoding the contents of the RPC payload.
...
 * @throws SerializationException if payload can't be deserialized
 * @throws ClassNotFoundException if requested interface can't be
 *   loaded from RPC.getClass().getClassLoader()
 * @throws NoSuchMethodException if requested method can't be found
 *   on requested interface
 */
public static RPCRequest decodeRequest(String payload) throws
SerializationException, ClassNotFoundException, NoSuchMethodException {
... }

/**
 * Returns a <code>RPCRequestParameters</code> class that is built by
 * decoding the contents of the RPC payload against a class that
 * constrains the set of valid service interfaces.
...
 * @throws SerializationException if payload can't be deserialized
 * @throws ClassNotFoundException if requested interface can't be
 *   found in constraintClass
 * @throws NoSuchMethodException if requested method can't be found
 *   on requested interface
 */
public static RPCRequest decodeRequest(String payload, Class
constraintClass) throws SerializationException, ClassNotFoundException,
NoSuchMethodException  { ... }

Absolutely, great.

I like this new proposal much better.

I agree with you that the invoke method isn't necessary in its current
form.  But I think there's another version of the invoke method that,
while not strictly necessary, is so immensely convenient as to be worth
having.  Read on.

Makes some amount of sense, but I think the isException is a bit
problematic and I'm not clear on why it's needed.  Seems to me you
could just do:

public String encodeResponse(Method serviceMethod, Object result)
throws SerializationException, InvocationTargetException {
  if (result instanceof Exception) {
    if (!isExpectedException(serviceMethod, result.getClass())) {
      throw new InvocationTargetException(result, ...);
    }
  }
  return createResponse(new ServerSerializationStreamWriter(...),
result);

This then throws InvocationTargetException for unexpected exceptions,
which I think is better than wrapping them in SerializationException.
IMO SerializationException should be reserved for exceptions thrown
from rpc.impl.

This makes good sense (except that isException is actually not set
properly here, which is one reason I think it's a bit tricky for the
developer to get right).

If we modify the decodeRequest and encodeResponse methods as per my
suggestions, then the exception signature of
RemoteServiceServlet.processCall becomes SerializableException,
IllegalAccessException, ClassNotFoundException, NoSuchMethodException,
InvocationTargetException.  This is a lot, but each one is clear.
(Reflection always has tons of exceptions flying around....)

An issue I see with this is that runtime exceptions thrown by the
service method will effectively get swallowed and converted into
InvocationTargetExceptions.  See George G.'s post above as to why
that's generally bad for most service containers, especially ones that
implement JTA.

It turns out that the above code is basically exactly what any server
framework will want to do, with the sole exception of invoking on
"this".  So I propose refactoring the above code into... you guessed
it... RPC.invoke() :-)

String invoke(String payload, Object target) throws
SerializableException, IllegalAccessException, ClassNotFoundException,
NoSuchMethodException, InvocationTargetException {
  RPCRequest rpcRequest = RPC.decodeRequest(payload,
target.getClass());
  Method serviceMethod = rpcRequest.getMethod();
  Object result;
  try {
    result = serviceMethod.invoke(target,
rpcRequest.getParameterValues());
  } catch (InvocationTargetException e) {
    result = e.getCause();
    if (result instanceof RuntimeException) {
      throw e;
    }
  }
  return RPC.encodeResponse(serviceMethod, result);

This rethrows service method RuntimeExceptions, which I think is the
cleanest place to do it.  It completely manages all details of encoding
and exception handling, while providing the flexibility to invoke any
given service object after any amount of routing, which was one of the
big original goals of this refactoring.  It even still supports the
progressive decoding so we don't deserialize any more than we need to!

And RemoteServiceServlet.processCall devolves to:

String processCall(String payload, HttpServletRequest request,
HttpServletResponse response) throws SerializableException,
IllegalAccessException, ClassNotFoundException, NoSuchMethodException,
InvocationTargetException {
  return RPC.invoke(payload, this);

If we do this, do we really even still want to expose decodeRequest and
encodeResponse?????  Just asking because it occurs to me to ask... for
now I'll assume so.

Done deal, then.

Here's a revised summary based on this post.

package com.google.gwt.user.server.rpc;

public static class RPCRequest {
  RPCRequest(Method serviceMethod, Object[] parameterValues) { ... }
  public Method getMethod()  { ... }
  public Object[] getParameterValues() { ... }

public class RPC {
/**
 * Returns an <code>RPCRequest</code> that is built by
 * decoding the contents of the RPC payload.
 *
 * NOTE: this method calls decodeRequest(String, Class) with null for
the
 * class parameter.
 *
 * @param payload the RPC payload encoded as a UTF-8 string
 * @return RPCRequest instance which contains all of the RPC request
 *   parameters
 * @throws SerializationException if can't decode the payload
 * @throws ClassNotFoundException if the request interface can't be
 *   loaded from RPC.getClass().getClassLoader()
 * @throws NoSuchMethodException if the request method can't be found
 *   in the request interface
 */
public static RPCRequest decodeRequest(String payload) throws
SerializationException, ClassNotFoundException, NoSuchMethodException {
... }

/**
 * Returns an <code>RPCRequest</code> class that is built by
 * decoding the contents of the RPC payload against a
...

read more »

On 1/25/07, Rob Jellinghaus <r...@unrealities.com> wrote:

Hey Rob,

Really sorry to hear about that, but thank you very much for effectively
writing your note twice.

Just so you (and everyone else on the list) knows Google Groups actually
doubles as a mailing list, so you can get all the messages sent to an email
address of your choosing and simply respond in your favorite mail reader
(which presumbly saves outbound messages locally). I've found that
integration into my regular email is much easier to manage than periodic
visits to a web forum and you can filter/label easily as well, but YMMV.

I believe you should be able to change your settings at:
http://groups.google.com/group/Google-Web-Toolkit-Contributors/subscribe

Now, back to your regularly scheduled code review,
-Dan

Hi Rob,

I'm sorry you lost your post man.  First, let me say that I think that we
are converging.  So much so, that I decided to check your CLA status.  It
does not appear that we have one on file for you.  Please be sure that you
follow the steps at Contributor License
Agreement<http://code.google.com/webtoolkit/makinggwtbetter.html#committers>.
per the originating contrib thread, Issue 389, RPC delegation support -
active?<http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse...>
Let me know if your CLA status is not correct.

I know that it can be painful to discuss APIs in this manner but this is
basically the same process that we go through internally when we work on an
API.

My comments are interleaved below with some content removed.  The rolled up
API is also listed at the end of this email.

On 1/26/07, Rob Jellinghaus <r...@unrealities.com> wrote:

[content removed]

Brilliant, makes perfect sense.  One issue, though: currently,doing the

I think that the interface cache should be moved into the RPC class as an
implementation detail.  The number of lookups can be significant for a
reasonably loaded system.

[content removed]

/**

You are correct we should throw separate exceptions for class not found or
no such method.

[content removed]

I agree with you that the invoke method isn't necessary in its current

There are a couple of reasons for having isException.  First, the stream
encodes additional information in the event that it is encoding a thrown
exception so that the client can know to call the onError method.  Second,
it is legal for someone to publish a service method that returns an
exception.  (Stylistic issues aside)  In that event you could not assume
that because the object is an instance of Exception that it was thrown.
Maybe renaming the parameter to wasThrown would clarify the intent.
However, in retrospect maybe I was too much of a reductionist.  If it makes
the API clearer, we could consider having one method for encoding results
and another for encoding thrown exceptions.  Something like

public String encodeResult(Object result) throws SerializationException {
... }
public String encodeThrownException(Method serviceMethod, Exception
exception) throws SerializationException, UnexpectedException { ... }

The encodeThrownException method is specified to throw an
UnexpectedException in the event that the actual type of the exception
argument is not listed in the service method's set of checked exceptions.
There is a java.rmi.UnexpectedException but we should probably create one in
the server's rpc package since the semantics are not identical.  Non-checked
exceptions would just be rethrown.

This then throws InvocationTargetException for unexpected exceptions,

Good catch and point. I had originally mocked up the code with two return
statements.  Then I decided to simplify it to one return statement and I
broke the code.  If you buy the encodeResult/encodeThrownException argument
then processCall could be written as follows:

String processCall(String payload, HttpServletRequest request,
HttpServletResponse response) throws SerializationException,
IllegalAccessException, UnexpectedException {
  RPCRequest rpcRequest = RPC.decodeRequest(payload, getClass());
  return RPC.invoke(rpcRequest.getMethod(), this,
rpcRequest.getParameterValues();

If we modify the decodeRequest and encodeResponse methods as per my

I think that your recommendation for adding more explicit exceptions to
decodeRequest and encodeResponse is a good one.

An issue I see with this is that runtime exceptions thrown by the

George's problem is a valid one.  However, the conversion of all
non-specified exceptions actually takes place in the doPost method of the
RemoteServiceServlet and it is not a function of the RPC utility class that
we have been discussing.  At least, as I understand the problem.  So, our
current RemoteServiceServlet will not play nice with the types of system's
that George is talking about.

If it did, I would think that the end result would be that the exception
escapes the servlet that contains the RPC functionality which means that the
client may not get a response from the server.  In that event, one of the
client's http request slots would remain used up.  If this happened several
times the client would eventually be unable to request data from the
server.  In order to fix that problem, you would need to be able to make RPC
requests and associate a timeout with them.  This is something that we are
looking to add in the next version of RPC.

It turns out that the above code is basically exactly what any server

...

read more »

On Jan 26, 8:46 am, "Miguel Méndez" <mmen...@google.com> wrote:

That is a shame, as I followed the instructions to the letter two weeks
ago.  I mailed it in by postal mail.

I just now faxed another one to the number on the CLA form.  At the
bottom of it I put a note requesting that they inform you when they
receive it.  The fax transmitted successfully, so if it doesn't appear
in the system within the expected timeframe for processing CLAs by fax,
definitely let me know!  (Hopefully I don't wind up with two in the
system, causing problems....)

I have no idea why you might think I'm experiencing any pain here :-)
This is great!

Roger, will do.

Check.

Aha, of course they could.  EVIL, EVIL, but they could.  I suppose it's
not really all that evil, actually.  Good catch!

Yes, that seems preferable.

That all works for me.  We would need to document encodeThrownException
carefully, to indicate that it rethrows RuntimeExceptions.  I'm fairly
convinced that doing so is the safest thing for it to do, and if the
developer doesn't like it they can catch such exceptions themselves in
their invoke logic.

Hm, I'm slightly confused here since this new version of processCall
doesn't explicitly call either encode method.  In other words, your
code sample doesn't seem any different here, hence I'm not clear on
what it illustrates.

Given your well-founded desire for an invoke(Method, Object, Object[])
overloading, and your well-taken point about rethrowing Errors, not
only RuntimeExceptions, I expect what you meant is:

String invoke(String payload, Object target) throws
SerializableException, IllegalAccessException, ClassNotFoundException,
NoSuchMethodException, UnexpectedException /*, RuntimeException, Error
*/ {
  RPCRequest rpcRequest = RPC.decodeRequest(payload,
      target.getClass());
  Method serviceMethod = rpcRequest.getMethod();
  return invoke(target, serviceMethod,
      rpcRequest.getRequestParameters());

String invoke(Object target, Method serviceMethod, Object[] parameters)
throws SerializableException, IllegalAccessException,
UnexpectedException /*, RuntimeException, Error */ {
  String responsePayload;
  try {
    Object result = serviceMethod.invoke(target,
        parameters);
    responsePayload = encodeResult(result);
  } catch (InvocationTargetException e) {
    Throwable thrown = e.getCause();
    if (thrown instanceof Exception) {
      // this will rethrow thrownException if it is a
      // RuntimeException; it will throw an
      // UnexpectedException if thrownException is not
      // expected by serviceMethod; otherwise, it will
      // encode thrownException as an error response
      responsePayload =
          encodeThrownException(serviceMethod, thrownException);
    } else {
      // thrown is an Error
      Error error = (Error)thrown;
      throw error;
    }
  }
  return result;

(Side question: does reflection actually wrap Errors in
InvocationTargetException?  I would have assumed it was only
Exceptions, but it doesn't hurt to check, I suppose... I've just never
seen InvocationTargetException-unwrapping code that checked for Errors
before.)

Hmm, interesting, very good points.  I tend to agree it would be more
optimal for the server to return an error response than to simply fail
to return at all.  It looks like in the case of unchecked exceptions
(the most problematic rollback case), the servlet would need to:

1) catch the unchecked exception
2) send the GWT RPC generic error response
3) rethrow the unchecked exception so the container's normal rollback
handling can work

However, it looks like we've already agreed that the methods of the RPC
class should throw runtime exceptions, so these details need to be (as
you say) left up to the servlet implementor / framework integrator.
Some good documentation in the RemoteServiceServlet class will clarify.
 I might also try to enhance the package documentation to clarify these
issues.

OK, that's fine.

[request / response RPC arguments]

Fair enough.

My very slightly revised API, based heavily on yours.  Basically my
only changes were making all the exception signatures consistent with
your post above (your API in your last post omitted
ClassNotFoundException and NoSuchMethodException from the decodeRequest
signatures, for example).  I'm not including full comments for each
@throws since I did that in my previous post.  Also, I'm proposing that
encodeThrownException also rethrows RuntimeExceptions, as above.  (This
is a bit controversial and I could go either way.)  Also, I swapped
Object and Method in the invoke() overloading, since (target, method,
params) seems more Java-like than (method, target, params).

public class RPCRequest {
  RPCRequest(Method serviceMethod, Object[] parameterValues) { ... }

  public Method getMethod() { ... }
  public Object[] getParameterValues() { ... }

public final class RPC {
  /**
   * Returns a <code>RPCRequest</code> class that is built by
   * decoding the contents of the RPC payload.
   *
   * NOTE: this method call decodeRequest(String, Class) with null for
the
   * class parameter.
   *
   * @param payload the RPC payload encoded as a UTF-8 string
   * @return RPCRequest instance which contains all of the RPC request
   *         parameters
   * @throws SerializationException, ClassNotFoundException,
NoSuchMethodException
   */
  public static RPCRequest decodeRequest(String payload) { ... }

   /**
   * Returns a <code>RPCRequestParameters</code> class that is built by
   * decoding the contents of the RPC payload against a class that
constrains
   * the set of valid service interfaces.
   *
   * @param payload the RPC payload encoded as a UTF-8 string
   * @param constraintClass if non-null the implementation checks that
this
   *      class actually implements the service interface requested in
the
   *      payload if it does not an exception is thrown
   * @return RPCRequest instance which contains all of the RPC request
   *      parameters
   * @throws
...

read more »

Two minor remarks:

- My concern about transaction management and exception handling was
directed more against implementations extending RemoteServiceServlet.
In an implementation (like the GWTHandler) that simply forwards RPCs
to a service embedded in the container framework this is of course not
an issue.

- I am not so enthusiastic about interface caches and interface
checking in the RPC classes. Factoring out RPC was on my wish list so
that I could wire with it service implementations that have no
dependencies on the GWT API. Many people have requested this for the
GWTHandler where you can map (almost) POJO beans to RPC. The problem
here is still that GWT requires that the delegate object implements
the RemoteService interface. While this makes sense when deciding
which methods to expose to RPC it forces services to implement that
interface. I think it's a fair and just burden for service exporters
like the GWTHandler to implement that security checking themselves.
But please don't wire it into the RPC module.

On Jan 27, 4:24 am, "g.georgovassi...@gmail.com"

Actually it is still an issue worth documenting for those use cases.  
But you leave RemoteServiceServlet between a rock and a hard place:  
should it throw unchecked exceptions or not?  If I were writing a top-
level servlet (not inside a JTA container), I would code it to catch
all exceptions at the top level.  That's the default assumption for
RemoteServiceServlet -- that it is the top-level servlet with no
context around it.

I wonder if it should do the 1) catch unchecked exception, 2) respond
with generic error, 3) rethrow unchecked exception, even if it is
totally unwrapped... that might actually be OK since it would cause
Tomcat to do its generic error logging as usual when an unchecked
exception escapes a servlet.  Thoughts?

What if it were an optional flag?  RPC.invoke(Object target, Method
method, Object[] parameters, boolean checkRemoteService)?  That way,
the RPC class could still do the interface and method existence
checking (which really is only the same checking any reflective
invocation has to do -- heck, it now uses the same exceptions even!),
and then optionally could check for RemoteService in the default
RemoteServiceServlet use case.  Perhaps it should throw
NotRemoteServiceException in the case where the delegate object isn;t
a remote service AND the checkRemoteService flag is set?

Great feedback, thanks George! :-)
Cheers!
Rob

Oops --

On Jan 27, 6:28 am, "Rob Jellinghaus" <r...@unrealities.com> wrote:

What I mean, of course, was:

RPC.decodeRequest(String requestPayload, boolean checkRemoteService)
RPC.decodeRequest(String requestPayload, Class constraintClass,
boolean checkRemoteService)
RPC.invoke(String requestPayload, boolean checkRemoteService)

The RPC.invoke(Object, Method, Object[]) overloading absolutely
DOESN'T need this new parameter :-)

Incidentally, I agree that enforcing the RemoteService marker
interface is problematic for more or less the same reasons as
enforcing IsSerializable.  Framework-specific marker interfaces are
always trouble for POJOs (that's why Hibernate moved away from them
entirely with version 3).

Cheers!
Rob

Ah, yes, Hibernate, another great love of mine :)
But I'm getting carried away, back to the main course. About exception
handling: at the servlet level it doesn't really make any difference,
JTA should in any decent application design wrap the services/
managers, not the controller (and thus the view, if we are talking
about servlets). So yes, at the servlet level one should serialise
SerializableException and regularly throw back to the container all
other exceptions/errors.

About your proposal to optionally check for the RemoteService
interface: if you could also factor that out to an interface like:

RPC.decodeRequest(String requestPayload, RPCAccessValidator
validator);

where

interface RPCAccessValidator{

boolean allows(Class target, String methodName, Class[] arguments);

then you may not only gain flexibility but also some speed, because
the JVM doesn't have to evaluate the checkRemoteService flag all the
time.

As for accessing the knownImplementedInterfaces: from a performance
point of view it may be beneficial to populate it during the service
initialisation and then only access it for reading (which, for the
concrete Set implementation you use, is allowed without
synchronisation).

<out of topic>
And since we are talking about about performance a minor sidenote on
something I noticed: in the RPC class there is a HashMap mapping type
names to classes. If an array is used instead that maps the toChar()
value of the type to the class, then you get an impressive gain. A
micro benchmark I wrote is 3 times faster than the hashmap lookup on
an 1.6 server JVM. A short extract (for simplicity I omit the static/
final):

Class[] TYPES = new Class[255];

static{
TYPES['Z'] = boolean.class;
TYPES['B'] = byte.class;
...

Class getType(String type){
return TYPES[type.charAt(0)];

</out of topic>

On Jan 27, 10:11 am, "g.georgovassi...@gmail.com"

Ditto :-)  In fact all of this is really in aid of getting GWT to play
very nicely with Seam.  I hope that the JDK 5 support lands in GWT 1.4
along with this RPC refactoring... then all the pieces will be in
place.

I think I agree.  And this makes me wonder something  Miguel, above,
you said:

But this isn't the behavior I see whenever a servlet I've worked on
throws an internal error.  The client does get a response --
specifically, a 500 Server Error response.  That's Tomcat's behavior
when exceptions escape a servlet.  Is that not in some sense the most
correct response, even more correct than the GWT GENERIC_FAILURE_MSG?  
Or will a 500 error not be received properly by XmlHttpRequest in most
browsers, at least not in a form that GWT's web mode RPC can handle?

If the intended GWT RPC contract is that the server do its best to
avoid sending 500 errors, instead returning 200 with
GENERIC_FAILURE_MSG, then that's something we'll want to document very
well indeed, because it's definitely not obvious on the face of it.

Are you saying that because the JVM would effectively inline the
RPCAccessValidator call?  If it didn't, then certainly a boolean would
be more efficient.  I'm not sure how much of a difference this would
make in other than microbenchmark terms, though, and I'm also not sure
what other kinds of validators one might want than just "instanceof
RemoteService", so it's not clear to me this is worth the code
complexity.

Interesting optimizations, but again I'm not sure those particular
operations are likely to be the hotspots.  The recursive superclass
walking and reflection code, on the other hand, more probably is.  I
am leaning towards not doing these optimizations in this pass, simply
because I would rather land this basic refactoring ASAP; a subsequent
performance pass (which I would want to run with JDK 6 profiling over
the entire server.rpc layer) could address any bottlenecks in a data-
driven way.

Measure before optimizing is my new mantra, since last month when I
spent three weeks having a profiler teach me that I was an incredibly
naive Hibernate user :-)  But if Miguel likes them, I'm willing to be
overruled.  Miguel, what's your opinion?

Cheers!
Rob

OK, I just asked you:

On 27 Jan, 15:24, "Rob Jellinghaus" <r...@unrealities.com> wrote:

Then you invited me to the "gwt-sl" Google group, where I found this
post:

http://groups.google.com/group/gwt-sl/browse_thread/thread/
8d16619ba6dd1f98/?hl=en-GB#

There, you say:

And you then present at least two different techniques for doing
remote method validation :-)  Thereby answering my exact question
above!

So, I'm inclined to support your suggestion, though I would prefer
this:

interface RPCAccessChecker {
   /**
    * Check the accessibility of the given method on the target class,
    * using whatever technique is appropriate for the service
framework.
    * @throws IllegalAccessException if the method should not be
    * accessible on the class.
    */
   public void checkAccess (Class targetClass, Method method) throws
IllegalAccessException;

I think passing a Method is better than passing a String method name
and a parameter array, because the method lookup is independent of
(and prior to) the access check.

Then the signatures of the RPC methods would change to:

public RPCRequest decodeRequest (String payload, RPCAccessChecker
accessChecker) throws SerializationException, ClassNotFoundException,
NoSuchMethodException, IllegalAccessException;

public RPCRequest decodeRequest (String payload, Class
constraintClass, RPCAccessChecker accessChecker) throws
SerializationException, ClassNotFoundException, NoSuchMethodException,
IllegalAccessException;

public String invoke (Object target, String requestPayload,
RPCAccessChecker accessChecker) throws SerializationException,
ClassNotFoundException, NoSuchMethodException, IllegalAccessException,
IllegalArgumentException, UnexpectedException;

And there would be a default implementation of RPCAccessChecker:

public final class RPCRemoteServiceAccessChecker {
  public RPCRemoteServiceAccessChecker() { }
  public void checkAccess (Class constraintClass, Method method) {
    if (!method.getClass() instanceof RemoteService) {
      throw new IllegalAccessException("Cannot access method " +
method + "; it is not subclassed from RemoteService");
  }

Then RemoteServiceServlet.processCall would be:

public String processCall (String requestPayload, HttpServletRequest
request, HttpServletResponse response) throws SerializationException,
ClassNotFoundException, NoSuchMethodException, IllegalAccessException,
IllegalArgumentException, UnexpectedException {
  return RPC.invoke(this, requestPayload, new
RPCRemoteServiceAccessChecker());

How's that?
Cheers!
Rob

I think adding an accessChecker parameter to any public method is wrong.

THe primary purpose of Rob's work is to create an abstraction which
executes rpc requests. The interface should only include the minimum
to execute a request and produce a response payload. The interface
should not be attempting to also answer application service security.

Features such as access checking are an implementation detail and
should not polute the interface. So many Eg EJB methods dont include
an AccessChecker parameter. Access checking probably also requires
some context - eg the user principal.

I would definitely remove the access checker parameter. If the service
wants some access checking they can include an appropriately mapped
filter or decorate the service etc.

On 1/28/07, Rob Jellinghaus <r...@unrealities.com> wrote:

--
mP

On 27 Jan, 21:35, "Miroslav Pokorny" <miroslav.poko...@gmail.com>
wrote:

It's a good point and a good perspective.  In my last post I posited
adding this checking to the decodeRequest methods.  You've convinced
me that that would be an error -- those should *just* be responsible
for decoding the request, *not* for checking service security.

Unfortunately this also makes it fairly clear that my convenience
invoke method -- public String invoke (Object target, String
requestPayload) -- is caught between two use cases.  In the main GWT
case, that method would *have* to check that the service interface
subclasses RemoteService.  But in my G4JSF case, that check is
undesirable, for the same reasons George doesn't want to enforce it.  
So the issue devolves to whether or not to perform the original
check.  We're back to:

decodeRequest methods as in my last major post:

public RPCRequest decodeRequest (String requestPayload) throws ...;
public RPCRequest decodeRequest (String requestPayload, Class
constraintClass) throws ...;

and the convenience overloading of invoke (only) has the
checkRemoteService boolean:

public String invoke (Object target, String payload, boolean
checkRemoteService) throws ...;

As you say, if a service requires some custom security checking, they
can do it either by interposing on the target object or by simply
writing their own implementation of RPC.invoke(Object, String) --
which will certainly be not much more than the six or so lines of code
that the base version is.

Thanks for the input, Miroslav!  Since we've now iterated a whole lot,
I will work on having a revised patch -- with just the
RemoteServiceServlet, (simple bean) RPCRequest, and (decodeRequest /
invoke / encodeResponse) RPC class -- by early next week.  That should
provide a clear basis for further discussion, since it's getting
painful to duplicate these signatures in each successive thread :-)

Cheers!
Rob

I agree with both of you that the access check in RPC is at the wrong
place - the only reason I proposed it there was because (I thought)
you desired for performance reasons a way to quickly reject an illegal
access. Thus, one could reject bad access calls by only looking at the
RPC 'header', possibly without further decoding the entire payload...

On Jan 28, 1:39 am, "g.georgovassi...@gmail.com"

Makes sense, and you were right.  Subsequent discussion, though, seems
to indicate that security checking is overall best done in other
layers of the system -- as is in fact the case in most other areas
(existing EJB3 security checking, for instance).  So performance
likely isn't worth violating proper layering.

The existing pattern still checks for interface and method existence
prior to complete decoding, so at least it avoids some corrupt message
scenarios.

I will document that the possibility exists to add security checking
to the decode methods if early security violation detection (prior to
full decoding) becomes a pragmatic necessity.  It could be added as an
additional decodeRequest (and invoke) overloading without breaking
backwards compatibility.

Cheers!
Rob

Hi Rob,

I just received confirmation that we have your CLA.  So we are good on that
front.

I don't think that we need the RPC.invoke(String, Object) overload.  It only
saves a single call to RPC.decodeRequest.  My original point was that
processCall could be significantly simpler as a result of the RPC
refactoring.  Good catch on the missing exception.

On 1/26/07, Rob Jellinghaus <r...@unrealities.com> wrote:

...

read more »