I like the cleanup you've done here - however, there are a few things that I'd like you to take a look at before I give the LGTM. Let's go for one more round on this one; we can discuss this stuff f2f if it's easier.
Side note: When these changes to MessageTransport land, we should update GPE so that it uses this newer version of MessageTransport as well; it's not a requirement, but it's nice to keep them somewhat in sync. http://gwt-code-reviews.appspot.com/550801/diff/1/2 File dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java (right): http://gwt-code-reviews.appspot.com/550801/diff/1/2#newcode99 dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java:99: callback.onError(e); Nit: You need to synchronize around the callback object, because its fields are being set by different threads - no, I'm wrong, you don't, because your callback calls methods on future, and those are protected. It would be good to note this in the comment though, since it it is not generally known whether or not the callback does its own synchronization or not - or you could doc the Callback interface to indicate that implementers of the interface must ensure thread safety themselves. http://gwt-code-reviews.appspot.com/550801/diff/1/2#newcode118 dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java:118: callback.onDone(responseMessage); Nit: You need to synchronize around the callback object, because its fields are being set by different threads - no, I'm wrong, you don't, because your callback calls methods on future, and those are protected. It would be good to note this in the comment though, since it it is not generally known whether or not the callback does its own synchronization or not - or you could doc the Callback interface to indicate that implementers of the interface must ensure thread safety themselves. http://gwt-code-reviews.appspot.com/550801/diff/1/2#newcode240 dev/core/src/com/google/gwt/dev/shell/remoteui/MessageTransport.java:240: class FutureTaskExtension extends FutureTask<Response> { Nice simplification (and removal of unnecessary asynchronousity/queuing) From reading the java docs, it seems that calling future.get(..) will block until: 1) an exception is set on the future 2) future.set(..)is called 3) the thread calling future.get(..) is interrupted The callable is executed whenever future.get(..) is called, but we don't need to worry about having the callable set the future's value; the future will still block until future.set(..) is called. Do I have this right? http://gwt-code-reviews.appspot.com/550801/diff/1/4 File dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceClient.java (right): http://gwt-code-reviews.appspot.com/550801/diff/1/4#newcode49 dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceClient.java:49: public void onError(Throwable t) { Error handling? In the addLogEntry case, a potential error is being lost. We at least need to log this to the console (which is weird, I know, since we're in a logger abstraction right now), or we need to figure out a way to throw this exception out via the main thread. Maybe we release the lock on which the main thread waits if we hit one of these exceptions? http://gwt-code-reviews.appspot.com/550801/diff/1/5 File dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceTreeLogger.java (right): http://gwt-code-reviews.appspot.com/550801/diff/1/5#newcode75 dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceTreeLogger.java:75: private volatile int logHandle = -1; Nit: volatile works, but I prefer AtomicInteger for clarity. http://gwt-code-reviews.appspot.com/550801/diff/1/5#newcode110 dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceTreeLogger.java:110: pending = Lists.add(pending, new PendingBranch(child, type, msg, caught, You need synchronization around the pending object here. The pending variable is non-final, and it's being re-assigned here. http://gwt-code-reviews.appspot.com/550801/diff/1/5#newcode121 dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceTreeLogger.java:121: pending = Lists.add(pending, new PendingLog(indexOfLogEntry, type, msg, You need synchronization around the pending object here. The pending variable is non-final, and it's being re-assigned here. http://gwt-code-reviews.appspot.com/550801/diff/1/5#newcode126 dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceTreeLogger.java:126: synchronized void initLogHandle(int result) { Rename the param to "newLogHandle" http://gwt-code-reviews.appspot.com/550801/diff/1/5#newcode129 dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceTreeLogger.java:129: for (Pending item : pending) { Create an instance lock and use it to protect the assignment or logHandle,the iteration over the pending list, and the possible re-init of the pending variable; then use the same lock to protect the assignment of the pending variable in the two methods above. http://gwt-code-reviews.appspot.com/550801/diff/1/5#newcode144 dev/core/src/com/google/gwt/dev/shell/remoteui/ViewerServiceTreeLogger.java:144: public void onError(Throwable t) { We need to do something with this error. It's being lost here. Before, we threw unchecked exceptions, which would end up terminating DevMode, since they would flow all the way back. We at least need to log this to the console (which is weird, I know, since we're in a logger abstraction right now), or we need to figure out a way to throw this exception out via the main thread. Maybe we release the lock on which the main thread waits if we hit one of these exceptions? http://gwt-code-reviews.appspot.com/550801/diff/1/7 File dev/core/test/com/google/gwt/dev/shell/remoteui/MessageTransportTest.java (right): http://gwt-code-reviews.appspot.com/550801/diff/1/7#newcode69 dev/core/test/com/google/gwt/dev/shell/remoteui/MessageTransportTest.java:69: Socket serverSocket = listenSocket.accept(); Yeah, not sure why we did all that kung-fu there....;). Nice cleanup. Okay, I remember now - to be "technically correct", we wanted to have the server socket in "accept" mode before any client connections could be made. In practice (for this), the the socket implementation on all platforms keeps a buffer for pending connections that have not been accepted as yet, so we're fine. http://gwt-code-reviews.appspot.com/550801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
