Sorry that it took me a little while to review this code. I had to
refresh my memory on some of the JRE's built-in concurrency features.
That said, I think that I have two overall comments:
1) We should be able to simplify MessageProcessor if we reused some of
the JRE concurrency support.
2) Shouldn't there be a result (if not a Response) for every Request?
Otherwise, couldn't a client hang if a single request on the server
crashes?
If you agree then, I'd propose the following API for MessageTransport:
class MessageTransport {
/*
* Returns the next available request, blocks the caller if one is
not available.available
*/
public Request nextRequest() ...
/*
* Initiates a request send. Notes that this returns a Future which
a caller can use to poll for the response, or block until the response
arrives. Also provides a mechanism for reporting back
* exceptions.
*/
public Future<Response> sendRequest(Request request) ...
}
That API could be implemented using an ExecutorService for sending
(gives you Futures for "free") and a thread for reading
requests/response from the input stream. The Callable passed to the
ExecutorService should be able to use a Condition JRE object to wait for
the input thread to signal it when it sees its associated response. If
the input thread sees a request it could simply queue it into a
LinkedBlockingQueue and leave it there.
MessageProcessor could then become more of a MessageDispatcher or
RequestDispatcher. It could pull the next request and dispatch it to the
correct service.
http://gwt-code-reviews.appspot.com/81805/diff/1/2
File
dev/core/src/com/google/gwt/dev/shell/remoteui/MessageProcessor.java
(right):
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode52
Line 52: public static interface RequestHandler {
FWIW, you might be able to get rid of this interface if you simply leave
requests in a queue that a called could pull from.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode71
Line 71: private static class ResponseWaiter {
I think that this class could be replaced with the JRE's Future<T>
class.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode143
Line 143: public void sendMessage(Message.Request requestMessage) throws
IOException {
As I've been looking at the code, its occurred to me that we must always
have a response. Otherwise a request could hang forever if the request
handler on the other side of the protocol crashes.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode167
Line 167: public Message.Response sendMessageAndWaitForReturn(
I think that you really want to use an ExecutorService here. It will
give you a Future<Response> that a caller can choose to block on or poll
for a result. Also, a Future<Response> gives you a way to pass
exceptions back to the original caller without having to write so much
code.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode200
Line 200: private int allocateNextMessageId() {
I think that you should use AtomicInteger here. That JRE class will
allow to you get rid of this method and the associated locks.
http://gwt-code-reviews.appspot.com/81805/diff/1/2#newcode221
Line 221: private void startMessageReceiverThread() {
It might be cleaner to simply queue requests and let the called pull
them from a LinkedBlockingQueue.
http://gwt-code-reviews.appspot.com/81805/diff/1/3
File
dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java
(right):
http://gwt-code-reviews.appspot.com/81805/diff/1/3#newcode46
Line 46: private final BufferedOutputStream out;
I don't think that you need to buffer the socket streams since the
Message.parseXXX and Message.writeXXX buffer internally.
http://gwt-code-reviews.appspot.com/81805/diff/1/3#newcode63
Line 63: out = new BufferedOutputStream(socket.getOutputStream());
I don't believe that the buffering is necessary here.
http://gwt-code-reviews.appspot.com/81805/diff/1/3#newcode91
Line 91: private byte[] receive() throws IOException {
I just noticed that, for java, protoc will generate utility methods on
the message classes for reading and writing length delimited messages
into/out of the streams. What's even better is that the length is
written using the streams int32 encoding. You should use those methods
and delete the receive and send methods.
http://gwt-code-reviews.appspot.com/81805
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
