Code Review Requested: issue #389, server.rpc.RemoteServiceServlet refactoring
Rob Jellinghaus
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&id=389#c9
As discussed in this earlier thread:
http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_thread/thread/8f1140184070548f/#
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
g.georgo...@gmail.com
I'll look shortly into it. Thank you alot!
Miguel Méndez
I'm taking a look at this patch as well. I'll have my feedback for you shortly.
Cheers,
Rob Jellinghaus
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
g.georgo...@gmail.com
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.
Rob Jellinghaus
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?
> 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.
Good point, I'll look into that.
> I am not very fond of the idea to
> serialise back to the client anything else than an explicit
> SerializableException...
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?
> Good work! And I'm glad you did it :)
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
Rob Jellinghaus
all that IF the GWT devs approve my patch ;-) (which I don't take for
granted!)
Miguel Méndez
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 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 <ro...@unrealities.com> wrote:
--
Miguel
Rob Jellinghaus
No worries, one day turnaround is great :-)
> public final class RPC {
> public static RPCRequest decodeRequest(String payload)
> throws SerializationException { ... }
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.
> [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...
> public static String encodeResponse(Method serviceMethod, Object result,
> boolean isException) throws SerializationException { ... }
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.
> /**
> * Returns a string that encodes the result of invoking the service method
> * with the specified parameters....
> * @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 { ... }
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.)
> 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.
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.
> What do you think?
It all makes a lot of sense. Your turn :-)
Thanks very much for the careful review!
Cheers!
Rob
Miguel Méndez
My comments are interleaved below. Note, that I included a summary of the changes based Rob's feedback at the end of this email.
> Sorry that it has taken me longer to get this out.
No worries, one day turnaround is great :-)
> public final class RPC {
> public static RPCRequest decodeRequest(String payload)
> throws SerializationException { ... }
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.
Yes. The RPCRequest class simply encapsulates the request parameters.
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.
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.
*
* @return RPCRequest instance which contains all of the RPC request
* parameters
* @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.
*
* actually implements the service interface requested in the payload
* parameters
* @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
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.
Definitely, some of the stuff that is currently public to RPC needs to be hidden.
> [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...
> public static String encodeResponse(Method serviceMethod, Object result,
> boolean isException) throws SerializationException { ... }
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.
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:
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:
...
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);
...
}
> /**
> * Returns a string that encodes the result of invoking the service method
> * with the specified parameters....
> * @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 { ... }
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.)
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.
> 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.
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.
Design for Inheritance definitely has its place. It just felt like it was not strictly necessary in this case. I'm definitely on board with merging RemoteServiceServlet back into BaseServiceServlet.
Here is a summary of the classes based on this latest round of discussion. [All non-public members have been ecluded for clarity.]
public static class RPCRequest {
RPCRequest(Method serviceMethod, Object[] parameterValues) { ... }
public Method getMethod() { ... }
public Object[] getParameterValues() { ... }
}
package com.google.gwt.user.server.rpc;
public class RPC {
/**
* Static classes have no constructability.
*/
private RPC() {
}
/**
* Returns a <code>RPCRequestParameters</code> class that is built by
* decoding the contents of the RPC payload.
*
* 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
*/
/**
* 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
* 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 SerializationException
*/
/**
* Returns a string that encodes the results of an RPC call.
*
* @param serviceMethod the method whose result we are encoding
* @param result the object that we wish to send back to the client
* @param isException if true the object was really an exception thrown by the service method implementation
* @return a string that encodes the result of calling a service method
* @throws SerializationException if the result cannot be serialized
*/
public static String encodeResponse(Method serviceMethod, Object result, boolean isException) throws SerializationException { ... }
}
> What do you think?
It all makes a lot of sense. Your turn :-)
Thanks very much for the careful review!
Cheers!
Rob
No problem Rob. I hope that you do not mind the back and forth but I think that it is a good thing. Thanks for jumping in and helping out. You're up.
Cheers,
--
Miguel
Rob Jellinghaus
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:
> 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.
Makes sense, and it looks like we can even preserve the progressive
decoding given your constraintClass proposal below.
> 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.
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....)
> public static RPCRequest decodeRequest(String payload) throws
> SerializationException { ... }
...
> public static RPCRequest decodeRequest(String payload, Class
> constraintClass) throws SerializationException { ... }
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 { ... }
> If that sounds palatable then we could simplify RPCRequest to only contain
> the service method and the request parameter values.
Absolutely, great.
> 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.
I like this new proposal much better.
> 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.
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.
> 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);
> }
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.
> 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);
> }
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.
> I'm definitely on board with merging
> RemoteServiceServlet back into BaseServiceServlet.
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 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, ClassNotFound is thrown
* @return RPCRequest describing the request that was decoded
* @throws SerializationException if can't decode the payload
* @throws ClassNotFoundException if the request interface can't be
* found in constraintClass
* @throws NoSuchMethodException if the request method can't be found
* in the request interface
*/
public static RPCRequest decodeRequest(String payload, Class
constraintClass) throws SerializationException, ClassNotFoundException,
NoSuchMethodException { ... }
/**
* Returns a string that encodes the results of an RPC call.
*
* @param serviceMethod the method whose result we are encoding
* @param result the object that we wish to send back to the client
* @return a string that encodes the result of calling a service method
* @throws SerializationException if the result cannot be serialized
* @throws InvocationTargetException if the result was an unexpected
* exception (one not declared in the serviceMethod's signature)
*/
public static String encodeResponse(Method serviceMethod, Object
result) throws SerializationException, InvocationTargetException { ...
}
/**
* Decodes the request, validates it against the provided target
object,
* invokes the service method, and encodes the response, returning the
* encoded response payload.
*
* @param requestPayload the request payload
* @param target the object that implements the service
* @return String encoded RPC response, or encoded exception if the
* target object threw an exception that is declared in the request
* method's signature
* @throws SerializationException if the request cannot be
* deserialized, or the response cannot be serialized
* @throws ClassNotFoundException if the request interface can't be
* found in target.getClass()
* @throws NoSuchMethodException if the request method can't be found
* in the request interface
* @throws IllegalAccessException if the request method is not
* accessible on the target object
* @throws RuntimeException if the target object throws a
* RuntimeException; the exception will be the one thrown by the
* target object
* @throws InvocationTargetException if the target object throws an
* unexpected checked exception; the target object's exception will
* be the cause of the InvocationTargetException
*/
public static String invoke (String requestPayload, Object target)
throws SerializationException, ClassNotFoundException,
NoSuchMethodException, IllegalAccessException,
InvocationTargetException { ... }
}
I really think this may be the best of all possible worlds. (How
Panglossian!)
The ONLY remaining suggestion I have is this: all these methods pass
Strings as payload arguments. Would it make more sense to pass
Streams? Then we could even avoid the overhead of decoding the entire
request payload as a String -- if we found an invalid interface, we
would never even finish reading the entire request body from the
HTTPServletRequest! We could also pass an output stream into invoke
and encodeResponse, which would potentially also avoid some buffering
in the case of large responses. Is this a good idea, or is this
overkill? (It certainly wouldn't work with the existing logic that
decides whether or not to gzip the response... maybe we just do it for
the request?)
> No problem Rob. I hope that you do not mind the back and forth but I think
> that it is a good thing.
Hopefully you can see that I agree :-)
> Thanks for jumping in and helping out.
My pleasure. Are we converging?
Cheers!
Rob
Dan Peterson
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!
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
Miguel Méndez
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. per the originating contrib thread, Issue 389, RPC delegation support - active? 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.
About whether to deserialize inside RPCRequest:
[content removed]
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....)
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]
/**
* 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 { ... }
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
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.
> 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);
> }
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);
}
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,
which I think is better than wrapping them in SerializationException.
IMO SerializationException should be reserved for exceptions thrown
from rpc.impl.
> 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);
> }
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).
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:
}
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....)
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
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.
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.
I would leave them exposed since they afford the maximum amount of flexibility in the API yet they do not have excessive surface area. You would not need to use our version of invoke, etc. If you wanted to add the invoke(String,Object) method to the RPC class, I would definitely add an overload that invoke(Method, Object, Object[]).
I really think this may be the best of all possible worlds. (How
Panglossian!)
The ONLY remaining suggestion I have is this: all these methods pass
Strings as payload arguments. Would it make more sense to pass
Streams? Then we could even avoid the overhead of decoding the entire
request payload as a String -- if we found an invalid interface, we
would never even finish reading the entire request body from the
HTTPServletRequest! We could also pass an output stream into invoke
and encodeResponse, which would potentially also avoid some buffering
in the case of large responses. Is this a good idea, or is this
overkill? (It certainly wouldn't work with the existing logic that
decides whether or not to gzip the response... maybe we just do it for
the request?)
Personally, I would leave them as strings. It is simpler and avoids adding all of the additional failure modes from Streams into the API. So, here is the latest proposed API.
RPCRequest(Method serviceMethod, Object[] parameterValues) { ... }
public Method getMethod() { ... }
public Object[] getParameterValues() { ... }
}
/**
* Returns a <code>RPCRequest</code> class that is built by
* decoding the contents of the RPC payload.
*
* 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) { ... }
/**
* Returns a <code>RPCRequestParameters</code> class that is built by
* 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
* @return RPCRequest instance which contains all of the RPC request
* parameters
* @throws SerializationException
public static RPCRequest decodeRequest(String payload, Class constraintClass) { ... }
/**
* Returns a string that encodes the result object of an RPC call.
* @param serviceMethod the method whose result we are encoding
* @param result the object that we wish to send back to the client
* @return a string that encodes the result of calling a service method
* @throws SerializationException if the result cannot be serialized
public static String encodeResult(Object result)
throws SerializationException { ... }
/**
* Returns a string that encodes an exception thrown by a service method.
*
* @param serviceMethod the method that threw the exception
* @param exception the exception that was thrown by the service method
* @return a string that encodes the exception thrown by the method
* @throws SerializationException if the exception object cannot be serialized
* @throws UnsupportedException if the exception is not one of the checked
* exceptions listed by the method
*/
public static String encodeThrownException(Method serviceMethod,
Exception exception) throws SerializationException, UnexpectedException { ... }
/**
* Returns a string that encodes the result of calling a service method.
*
* Note, this method rethrows runtime exceptions.
*
* @param serviceMethod the method to invoke the RPC call.
* @param object instance on which to call the method
* @param parameterValues parameter to pass as part of the call
* @return a string encoding the result of invoking the service method
*
* @throws IllegalAccessException
* @throws IllegalArgumentException
* @throws SerializationException if an object could not be serialized by the stream
* @throws UnexpectedException if the serviceMethod throws a checked exception that does not match its list of checked exceptions
public static String invoke(Method serviceMethod, Object object,
IllegalAccessException, SerializationException, UnexpectedException {
try {
Object result = serviceMethod.invoke(object, parameterValues);
return encodeResult(result);
} catch (InvocationTargetException e) {
Throwable cause = e.getCause();
if (cause instanceof Exception) {
return encodeThrownException(serviceMethod, (Exception) cause);
}
Error error = (Error) cause;
throw error;
}
}
}
Miguel
Rob Jellinghaus
> 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.
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 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.
I have no idea why you might think I'm experiencing any pain here :-)
This is great!
> 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.
Roger, will do.
> You are correct we should throw separate exceptions for class not found or
> no such method.
Check.
> 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)
Aha, of course they could. EVIL, EVIL, but they could. I suppose it's
not really all that evil, actually. Good catch!
> 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.
Yes, that seems preferable.
> 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.
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.
> 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();
>
> }
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.)
> 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.
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.
> > If we do this [RPC.invoke()], 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.
>
> I would leave them exposed since they afford the maximum amount of
> flexibility in the API yet they do not have excessive surface area. You
> would not need to use our version of invoke, etc. If you wanted to add the
> invoke(String,Object) method to the RPC class, I would definitely add an
> overload that invoke(Method, Object, Object[]).
OK, that's fine.
[request / response RPC arguments]
> Personally, I would leave them as strings. It is simpler and avoids adding
> all of the additional failure modes from Streams into the API.
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 SerializationException, ClassNotFoundException,
NoSuchMethodException
*/
public static RPCRequest decodeRequest(String payload, Class
constraintClass) { ... }
/**
* Returns a string that encodes the result object of an RPC call.
*
* @param serviceMethod the method whose result we are encoding
* @param result the object that we wish to send back to the client
* @return a string that encodes the result of calling a service
method
* @throws SerializationException if the result cannot be serialized
*/
public static String encodeResult(Object result)
throws SerializationException { ... }
/**
* Returns a string that encodes an exception thrown by a service
method.
*
* Note, this method rethrows runtime exceptions.
*
* @param serviceMethod the method that threw the exception
* @param exception the exception that was thrown by the service
method
* @return a string that encodes the exception thrown by the method
* @throws SerializationException if the exception object cannot be
serialized
* @throws UnexpectedException if the exception is not one of the
checked
* exceptions listed by the method
* @throws RuntimeException if the exception is an unchecked, runtime
* exception
*/
public static String encodeThrownException(Method serviceMethod,
Exception exception) throws SerializationException,
UnexpectedException { ... }
/**
* Returns a string that encodes the result of calling a service
method.
*
* Note: this method rethrows runtime exceptions and errors thrown by
the target.
*
* @param serviceMethod the method to invoke the RPC call.
* @param object instance on which to call the method
* @param parameterValues parameter to pass as part of the call
* @return a string encoding the result of invoking the service
method
*
* @throws IllegalAccessException
* @throws IllegalArgumentException
* @throws SerializationException if an object could not be
serialized by
the stream
* @throws UnexpectedException if the serviceMethod throws a checked
exception that is not declared in its signature
*/
public static String invoke(Object target, Method serviceMethod,
Object[] parameterValues) throws IllegalArgumentException,
IllegalAccessException, SerializationException,
UnexpectedException { ... }
/**
* Decodes a request, invokes the service method on the given target,
and
* encodes and returns the response.
*
* Note: this method rethrows runtime exceptions and errors thrown by
the target.
*
* @param serviceMethod the method to invoke the RPC call.
* @param object instance on which to call the method
* @param parameterValues parameter to pass as part of the call
* @return a string encoding the result of invoking the service
method
*
* @throws IllegalAccessException
* @throws IllegalArgumentException
* @throws ClassNotFoundException
* @throws MethodNotFoundException
* @throws SerializationException if an object could not be
serialized
* or deserialized
* @throws UnexpectedException if the serviceMethod throws a checked
exception that is not declared in its signature
*/
public static String invoke(String requestPayload, Object target)
throws
IllegalArgumentException, IllegalAccessException,
SerializationException,
ClassNotFoundException, NoSuchMethodException,
UnexpectedException { ... }
}
Beautiful! I will start implementing this since we seem so close to
convergence that any remaining changes are likely to be proportionately
minor. I'll try to have a revised patch by the end of the weekend if
we are in fact converged.
Thanks enormously for the time and attention, Miguel, it's been a
pleasure. And if there are more iterations, fine!
Cheers!
Rob
g.georgo...@gmail.com
- 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.
Rob Jellinghaus
> - 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.
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?
> - I am not so enthusiastic about interface caches and interface
> checking in the RPC classes... 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.
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
Rob Jellinghaus
On Jan 27, 6:28 am, "Rob Jellinghaus" <r...@unrealities.com> wrote:
> What if it were an optional flag? RPC.invoke(Object target, Method
> method, Object[] parameters, boolean checkRemoteService)?
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
g.georgo...@gmail.com
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>
Rob Jellinghaus
> Ah, yes, Hibernate, another great love of mine :)
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.
> 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.
I think I agree. And this makes me wonder something Miguel, above,
you said:
> 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.
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.
> About your proposal to optionally check for the RemoteService
> interface: if you could also factor that out to an interface like:
>...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.
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.
> As for accessing the knownImplementedInterfaces: from a performance
> point of view it may be beneficial to populate it during the service
> initialisation...
> 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...
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
Rob Jellinghaus
On 27 Jan, 15:24, "Rob Jellinghaus" <r...@unrealities.com> wrote:
> 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.
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:
> Obviously you don't
> want every method to be exposed because your service might have
> setters or important 'secret' getters which must not be accessible
> through the web.
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
Miroslav Pokorny
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.
--
mP
Rob Jellinghaus
wrote:
> 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.
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
g.georgo...@gmail.com
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...
Rob Jellinghaus
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
Miguel Méndez
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.
Miguel Méndez
RemoteServiceServlet is definitely broken since it can swallow exceptions. We should file an issue against it. Subclasses should probably have a way of specifying that the exceptions should be allowed to escape into the servlet container? Seems like this is something that we should look into independently of this refactoring.
The only constraint that the RPC refactoring is placing is that the RemoteService interfaces which the client code calls, actually exist on the server's classpath. As long as you map the RPCRequest into a string that can be sent back to the client, you can do whatever you want.
In the delegation case, the instance that you delegate to does not need to actually implement the RPC interface. You can map the information contained in the RPCRequest instance onto a method invocation on a POJO using whatever mechanism/adapter is appropriate for your application. (This should be easier once we get rid of IsSerializable.)
Depending on how much mapping takes place you maybe able to use RPC.invoke(Object target, Method serviceMethod, Object[] parameterValues) to generate the result. Worst case you could always do the invocation yourself and simply use RPC.encodeResult, and RPC.encodeThrownException to build your response string. You could even get fancier and generate a interface that states that it inherits all of the target interfaces and get the early out behavior from RPC.decode(String, Class) method.
The net net is that as long as the RemoteService derived interfaces that are called are in the server's class path. You can always use as much or as little of the RPC utility class as a appropriate for your situation.
Does that make sense or did I misunderstand?
Rob Jellinghaus
Maybe it's me....]
On Jan 29, 12:56 pm, "Miguel Méndez" <mmen...@google.com> wrote:
> RemoteServiceServlet is definitely broken since it can swallow exceptions.
> We should file an issue against it. Subclasses should probably have a way
> of specifying that the exceptions should be allowed to escape into the
> servlet container? Seems like this is something that we should look into
> independently of this refactoring.
I'm contemplating doing something about it as part of this refactoring
simply because I'm munging RemoteServiceServlet so heavily anyway. I
can file another issue if you like, or would you like to?
Three questions about this:
1) If, right now, we changed RemoteServiceServlet to let unchecked
exceptions and UnexpectedExceptions escape the servlet, and the
container threw a 500 error, would the browser's XMLHTTPRequest
handler cope with the 500 error properly? And would the client-side
GWT RPC code treat it analogously to a GENERIC_FAILURE_MSG? (I'm not
proposing that we *do* this, I'm just asking what would happen.)
2) The simplest thing I could do to make this something that the
developer has control over would be to make the doPost method be non-
final. That's where the "catch everything and return
GENERIC_FAILURE_MSG" logic lives, and the developer could then
override it to let exceptions escape (or to send GENERIC_FAILURE and
*then* let exceptions escape, or whatever). OK with my doing that?
3) Do you know offhand whether RemoteServiceServletTest could feasibly
test the equivalent of a server 500 error? My guess is probably not,
at least not in hosted mode, but I figured I'd ask.... I don't think
there are any existing tests of RemoteServiceServlet's handling of
unchecked / unexpected exceptions (or even of expected exceptions!),
though please clue me in if I'm mistaken about that.
> The only constraint that the RPC refactoring is placing is that the
> RemoteService interfaces which the client code calls, actually exist on the
> server's classpath....
> The net net is that as long as the RemoteService derived interfaces that are
> called are in the server's class path. You can always use as much or as
> little of the RPC utility class as a appropriate for your situation.
>
> Does that make sense or did I misunderstand?
Speaking for George here (and for myself), the issue is that the
developer may have a pre-existing service interface -- perhaps even
one they don't have the source for -- that they want to expose to GWT
RPC. Forcing the interface to extend RemoteService is essentially
forcing one particular security policy on all developers. Spring, for
instance, supports multiple policies for service interposition -- for
example, you can declare that all methods matching some regexp should
be wrapped in a transaction. Likewise, the server developer might
have a regexp-based security policy instead of one based on
RemoteService.
My current patch (should be landing in issue 389 tonight) deals with
this by factoring out the RemoteService check from decodeRequest
altogether, and even from "String RPC.invoke(Object, Method,
Object[])". Instead, I've implemented the "String RPC.invoke(Object,
String, Class constraintClass, boolean checkRemoteService)"
overloading, which calls decodeRequest(), then
isImplementedRemoteServiceIntf() (if checkRemoteService is true), then
invoke(Object, Method, Object[]). This is now slightly more lines of
code -- just enough to be worth an overloading, I think :-)
Cheers!
Rob
Rob Jellinghaus
realizing that RemoteService is not optional -- the GWT client-side
compiler absolutely requires any RPC interfaces exposed over the wire
to inherit from RemoteService. So there is really no reason for
making the check for it be optional on the server side. Sorry,
George.
(Arguably this requirement is something that could be relaxed in some
way, but that is really and truly a separate issue and I won't roll it
into my patch for this one.)
I'm starting to think, though, that you could still get the behavior
you want. Say there's some proprietary interface you want to expose,
call it ProprietaryInterface. You can't make ProprietaryInterface
extend RemoteService.
But what you could do is define ProprietaryRemoteServiceInterface,
which extends ProprietaryInterface AND RemoteService. Then that's
what you put into your GWT component.
Then you make your RemoteServiceServlet subclass map method calls on
ProprietaryRemoteServiceInterface into the corresponding calls on
ProprietaryInterface. Something like (this is loose):
RPCRequest request = RPC.decodeRequest(requestPayload, ...);
// get the method that's named like the original, on the proprietary
interface
Method proprietaryMethod =
proprietaryServiceClass.getMethod(request.getMethod().getName(),
request.getMethod().getParameterTypes());
Object result = proprietaryMethod.invoke(target,
request.getParameters());
// encode the result using the GWT RPC version
String responsePayload = RPC.encodeResult(request.getMethod(),
result);
You'd have to get the exception handling right, but such is life.
It's a bit wonky, but not really that bad, and it works generically
for any set of interfaces you want to expose this way.
So I'll revise my code accordingly. decodeRequest should and will
check for RemoteService itself, and I'll drop the RPC.invoke(Object,
String) overload as Miguel suggests.
Cheers!
Rob
Rob Jellinghaus
http://code.google.com/p/google-web-toolkit/issues/detail?id=389
The biggest (only?) change from the discussion to date is that in
implementing the various RPC.decodeRequest and RPC.invoke methods, I
realized that it was cumbersome to put all of the reflection
exceptions (ClassNotFoundException, NoSuchMethodException,
IllegalArgumentException, IllegalAccessException) into the
signatures. It made for significantly more complex catch clauses for
essentially no benefit -- there is nothing that the caller can really
do to recover.
The original code threw SecurityException in all the analogous cases;
I adopted that convention as well. This also allows the
SecurityExceptions thrown to be significantly more informative (since
much servlet logging doesn't log exception causes deeply). If there
is strong sentiment to change this, I certainly can. Opinions?
As always, all comments appreciated. (Especially Miguel's :-) )
Thanks very much to everyone who's participated in this thread!
Cheers!
Rob
Miguel Méndez
I'm pretty swamped today; I'll do the review tomorrow.
Thanks,
Miguel
Rob Jellinghaus
Cheers!
Rob
Miguel Méndez
I was unable to extract your patch file. I think that, going forward, it would be good to take your changes and create a single patch file which we can apply from the trunk directory. It will make the review iterations go faster.
I ran into quite a few checkstyle warnings. The styles are described at http://code.google.com/webtoolkit/makinggwtbetter.html#codestyle . If you use eclipse, you can check out the following document, http://google-web-toolkit.googlecode.com/svn/trunk/eclipse/README.txt which describes how to configure eclipse to automatically format your code and to make sure that you are in conformance with our style guidelines. I find this capability to be very valuable. You should be able to get your code to conform with our checkstyle guidelines fairly quickly.
Lets get that sorted out and then will continue with the iteration.
Cheers,
Rob Jellinghaus
> I was unable to extract your patch file. I think that, going forward, it
> would be good to take your changes and create a single patch file which we
> can apply from the trunk directory. It will make the review iterations go
> faster.
OK. I assume then I should update to the latest trunk before doing
this to maximize the chances it will work. Hope this isn't a Windows
line-ending problem or something....
What error did you encounter when extracting the patch file? And what
command do you use to do it? I keep a separate clean copy of the
trunk, so I'll try applying my own patch before I submit it. (Sorry
I'm a bit ignorant of SVN patching....)
> I ran into quite a few checkstyle warnings.
Gotcha. I will pick up the checkstyle file and probably run
checkstyle manually (or maybe IDEA has a Checkstyle plugin). I'll fix
all this up tonight and have a revised patch before midnight.
Cheers!
Rob
John Tamplin
OK. I assume then I should update to the latest trunk before doing
this to maximize the chances it will work. Hope this isn't a Windows
line-ending problem or something....
If you have your working copy where you are making the changes off the trunk, then after svn update; svn diff > patch.txt should generate a patch that will apply cleanly to the trunk.
The line endings should be whatever is native to the platform, so if you move the patch file across platforms you will need to use the -l option to patch, which ignores whitespace differences.
What error did you encounter when extracting the patch file? And what
command do you use to do it? I keep a separate clean copy of the
trunk, so I'll try applying my own patch before I submit it. (Sorry
I'm a bit ignorant of SVN patching....)
From the top of a clean checkout of trunk, patch -p0 <patch.txt -- if it has any merge problems, you will get .orig and .rej files as well as errors describing which patch hunks failed. You shouldn't get any unless conflicting changes were made to the trunk between the time you did your svn diff and when you updated your copy of trunk.
--
John A. Tamplin
Software Engineer, Google
Miguel Méndez
On Feb 1, 11:21 am, "Miguel Méndez" <mmen...@google.com> wrote:
> I was unable to extract your patch file. I think that, going forward, it
> would be good to take your changes and create a single patch file which we
> can apply from the trunk directory. It will make the review iterations go
> faster.
OK. I assume then I should update to the latest trunk before doing
this to maximize the chances it will work. Hope this isn't a Windows
line-ending problem or something....
What error did you encounter when extracting the patch file? And what
command do you use to do it? I keep a separate clean copy of the
trunk, so I'll try applying my own patch before I submit it. (Sorry
I'm a bit ignorant of SVN patching....)
I used patch -p0 < {name of your patch file}
Here is what it reported:
patching file RemoteServiceServlet.java
patch: **** malformed patch at line 292: // Store the request & response objects in thread-local storage.
No worries, we'll get it figured out.
> I ran into quite a few checkstyle warnings.
Gotcha. I will pick up the checkstyle file and probably run
checkstyle manually (or maybe IDEA has a Checkstyle plugin). I'll fix
all this up tonight and have a revised patch before midnight.
I'm pretty busy for the rest of the day and tomorrow. So, don't kill yourself trying to get it in by midnight.
Cheers!
Rob
--
Miguel
Rob Jellinghaus
I fixed up the checkstyle errors to the best of my ability. Lacking
Eclipse, I munged the user/build.xml file to add <fileset dir="src"/>
to the gwt.checkstyle task, which got the build to run Checkstyle over
all the files in user/src. Presto, a bunch of checkstyle errors in my
files (only) popped up. I fixed the majority of them (method
ordering, whitespace).
I don't understand two of the errors. First, and most annoying:
[checkstyle] C:\robj\dev\p4_robj-home-desktop2\gwt-dev-robj\trunk\user
\src\com\google\gwt\user\server\rpc\RPC.java:7: Line does not match
expected header line of ' * '.
This seems to be due to line-endings. I have triply verified that the
contents of the line are <space><asterisk><space>. But I am on
Windows, and the gwt-checkstyle.xml file has:
value="/*\n * Copyright 2006 Google Inc.\n * \n..."
So the only thing I can think of is that this is a line-ending issue,
which I'm not sure how to resolve locally.
Second:
[checkstyle] C:\robj\dev\p4_robj-home-desktop2\gwt-dev-robj\trunk\user
\src\com\google\gwt\user\server\rpc\RPC.java:240:33: Unable to get
class information for @throws tag 'SerializationException'.
I'm not at all sure why not. There's nothing different about the
import of SerializationException in this file, versus the other files
which import it. There may be some idiosyncrasy of the build process
that is causing Checkstyle to not see the class (since "ant clean
test" at the root of trunk runs without failure).
In any case, since I'm not sure I'm checkstyling in the same way that
you are, I'm going to take the liberty of submitting in this condition
-- if you see these errors too, please help me understand how to fix
them, and/or just fix them locally? I don't mean to hold up the
process with more round-trips over issues that seem to be platform-
specific or my-build-specific.
Thanks very much :-)
Cheers!
Rob
Miguel Méndez
I think that we are looking good overall. As far as your issue #1 is concerned, it is a whitespace difference (maybe line endings?). I just copied one from a known good file. I'm not seeing the Serialization problem that you mentioned.
My comments on the current state of the code are below.
RemoteServiceServlet.java
- line 252 - public void doPost(HttpServletRequest request, HttpServletResponse response)
- It was originally final and should probably stay that way.
- The javadoc states that the servlet swallows all exceptions and returns a code of 200. I think that this incorrect since respondWithFailure still uses a 500 code.
- Whether the method logs the error or not should probably be controlled via a protected method. Something like protected boolean shouldLogException(Throwable). The default would be no and a subclass could override the method using whatever strategy is appropriate.
- The catch blocks for SercurityException and UnexpectedException are not necessary since we have a catch block for Throwable.
- line 315 - public final String processCall(String)
- We should not make String processCall(String) final because it was not previously. This would break existing code.
- The list of checked exceptions could also break existing code. The UnexpectedException would cause existing code to catch it which would be another breaking API change. The SecurityException is okay since it is a RuntimeException. See the comment below on UnexpectedException.
- line 340 - public String processCall(String, HttpServletRequest, HttpServletResponse)
- I'm not sure that we need this method. A subclass could always ask for the request or the response objects from their processCall(String) method using the getThreadLocal[Request | Response] methods.
- line 24 - Does it really respond with a 200? See my comment on RemoteServiceServlet.java line 252 above.
- line 110 - public static RPCRequest decodeRequest(String, Class)
- Minor: I'm still not crazy about the name. I'm just not sure if a user could determine which overload to call based on the name alone. The method is more like decodeRequestConstrained, decodeRequestWithConstraint or decodeRequestAgainstService.
- line 198 - public static String encodeThrownThrowable(Method, Throwable)
- Minor: Looking at this with a fresh pair of eyes, the method name sounds awfully weird. Maybe it should be renamed to encodeFailure(Method, Throwable). I would argue that this would make for a better name and it would also make us consistent with the name of the AsyncCallback method that will be called.
- line 201: I do not think that we need to rethrow the exception in this case. We should let RPC.isExpectedException do its thing and the normal course will be to throw an UnexpectedException with the Throwable as its cause.
- line 181 - public static String encodeResult(Method, Object)
- Minor: This method should be renamed encodeSuccess(Object). I would argue that this would make for a better name and it would also make us consistent with the name of the AsyncCallback method that will be called.
- line 29 & 34 - Minor: You might consider making these fields final.
- If this exception is not a runtime exception and it can be thrown from processCall then we will introduce an API break. I think that reporting back an UnexpectedException to the caller of processCall is important. Any recommendations on how we could handle this?
Thanks,
Rob Jellinghaus
> I think that we are looking good overall. As far as your issue #1 is
> concerned, it is a whitespace difference (maybe line endings?). I just
> copied one from a known good file. I'm not seeing the Serialization problem
> that you mentioned.
Phew. Glad to hear it, I'd rather not thrash!
> My comments on the current state of the code are below.
I accept the majority of your comments and will implement them.
Expect a new patch tonight. (Trying to make sure to get this
finalized in the next couple of days so as not to even come close to
the GWT 1.4rc1 release timeframe.)
I will comment only on the issues that I don't agree with
completely :-) On the 200 vs 500 status thing: my bad, some last-
minute editing didn't make it back to my development tree.
> - line 252 - public void doPost(HttpServletRequest request,
> HttpServletResponse response)
> - It was originally final and should probably stay that way.
Is this a backwards compatibility issue? The main use for not making
doPost be final would be if the user did want some other servlet-level
functionality (custom logging, or who knows what), but probably
processCall is sufficient, so I'm OK with making it final. I just
wanted to understand more of your thinking here; I assume it's just
that the current doPost logic is unlikely to be very safely
overridable.
> - Whether the method logs the error or not should probably be
> controlled via a protected method. Something like protected boolean
> shouldLogException(Throwable). The default would be no and a
> subclass could
> override the method using whatever strategy is appropriate.
It seems to me there are two kinds of variation the user's likely to
want: should the exception be logged at the server (actually I'm not
clear why they would ever not want this?), and do runtime exceptions
escape the servlet. I was expecting you to request an extension point
more like "shouldThrowRuntimeException(RuntimeException)" which the
user could override if they wanted runtime exceptions to escape.
> - line 110 - public static RPCRequest decodeRequest(String, Class)
> - Minor: I'm still not crazy about the name. I'm just not sure
> if a user could determine which overload to call based on the
> name alone.
> The method is more like decodeRequestConstrained,
> decodeRequestWithConstraint or decodeRequestAgainstService.
I suggest decodeAndCheckRequest or decodeRequestAgainstServiceClass.
> - If this exception is not a runtime exception and it can be thrown
> from processCall then we will introduce an API break. I think that
> reporting back an UnexpectedException to the caller of processCall is
> important. Any recommendations on how we could handle this?
Make it a RuntimeException? That seems to both support letting it
escape from the servlet should the user want that (by overriding
shouldThrowRuntimeException()), and avoids breaking the existing
RemoteServiceServlet.processCall signature.
> Thanks,
Thank *you* :-)
Cheers!
Rob
Rob Jellinghaus
Rather than shouldLogException or shouldThrowRuntimeException, I
implemented a simple "protected void handleFailure(Throwable)" method,
which by default simply logs using getServletContext(). The developer
can subclass this, if they like, to do custom logging or even to
rethrow the exception. This seems more general while being equally
concise.
I also simplified ALL the catch clauses except for "catch (Throwable
e)" out of doPost, and removed a couple of unused parameters from some
of the static helper methods.
All else is as per your last post. Here's hoping this patch applies
as smoothly as the prior one -- I am doing my local source control
with Perforce, and it's having some weird interactions with
Subversion. Patch looks good on inspection, though.
Are we there yet? ;-)
Cheers!
Rob
Miguel Méndez
Whether we log or not depends on the volume of requests that we are servicing. By default, we do not want to log exceptions before calling respondWithFailure.
I think that you have a good idea with the handleFailure method. I would rename it to onUnhandledException(Throwable), to match the onAfterResponseSerialized et al methods, and have it be empty. A user could always subclass to get the behavior that they want. Also, make sure to update its documentation to match that of onAfterResponseSerialized, etc.
I would just rollback the respondWithFailure method to what it was originally. Basically, having it be public and static doesn't really buy us much. Also the refactoring allows IOException to escape and so we are forced to deal with it in doPost.
Let's just leave the decodeAndCheckRequest and decodeRequestAgainstServiceClass. methods as decodeRequest. We'll get feedback on that from the final review.
Make those changes, and I'll start the final review/submission process. At a minimum, I would expect that we will get feedback on the decodeRequest methods, the fact that UnexpectedException extends a RuntimeException.
Good job,
Miguel
Rob Jellinghaus
> Make those changes,
You got it -- no problem with any of those.
> and I'll start the final review/submission process.
!!!!! :-)
> At a minimum, I would expect that we will get feedback on the decodeRequest
> methods, the fact that UnexpectedException extends a RuntimeException.
No worries. All the feedback so far has just made it better, and it
doesn't seem to be diverging.
> Good job,
You, too!
Cheers!
Rob
Miguel Méndez
Rob Jellinghaus
The ajax4jsf team is quite open to my g4jsf changes, also, so it looks
like that will move right along now. Next up: GWT integration with
Seam. I will report progress on both fronts here.
Thanks again for your diligence, Miguel. I will continue to handle
the final review feedback as punctually as I can manage.
Cheers!
Rob
Miguel Méndez
I added some unit tests and found a couple of bugs. I went ahead and fixed them. The code review email to GWTC will go out shortly.
Cheers,