I think this still needs some work. I'd suggest that we meet up and discuss the general threading pattern here; I think there could be a few pitfalls, so let's discuss the way that we want this class to work, and then re-work the threading/synchronization around that.
http://gwt-code-reviews.appspot.com/264801/diff/1/2 File user/src/com/google/gwt/junit/RunStyleSelenium.java (right): http://gwt-code-reviews.appspot.com/264801/diff/1/2#newcode118 user/src/com/google/gwt/junit/RunStyleSelenium.java:118: private final Object seleniumLock = new Object(); This also controls access to the "stopped" variable, right? If so, document it. http://gwt-code-reviews.appspot.com/264801/diff/1/2#newcode157 user/src/com/google/gwt/junit/RunStyleSelenium.java:157: Runtime.getRuntime().addShutdownHook(new Thread() { Ugh, it may just be me, but I'd hate to rely on shutdown hooks here. Can we just have a cleanup step (maybe part of a shell script that runs on the machine) that kills outstanding selenium sessions at the end of a run/before the start of a new run? http://gwt-code-reviews.appspot.com/264801/diff/1/2#newcode180 user/src/com/google/gwt/junit/RunStyleSelenium.java:180: public synchronized void launchModule(String moduleName) { Not sure if it's a good idea to have a synchronized method, and then the seleniumLock on the inside here. That could lead to deadlock. Why is this method synchronized in the first place? http://gwt-code-reviews.appspot.com/264801/diff/1/2#newcode236 user/src/com/google/gwt/junit/RunStyleSelenium.java:236: private synchronized boolean doKeepAlives() { Not sure if it's a good idea to have a synchronized method, and then the seleniumLock on the inside here. That could lead to deadlock. Why is this method synchronized in the first place? Are you protecting the remotes variable? Why not use a separate lock for that? http://gwt-code-reviews.appspot.com/264801/diff/1/2#newcode257 user/src/com/google/gwt/junit/RunStyleSelenium.java:257: synchronized (wasInterruptedLock) { Be careful here - you could run into deadlock - you're holding seleniumLock, and trying to acquire wasInterruptedLock. Do you really need to still be holding seleniumLock at this time? http://gwt-code-reviews.appspot.com/264801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe from this group, send email to google-web-toolkit-contributors+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
