Thanks Stuart
I changed ReadTimeoutTest.java only apply CountdownLatch part. Please
review.
http://cr.openjdk.java.net/~tyan/JDK-7168267/webrev.02/
Thank you
Tristan
On 12/20/2013 10:47 AM, Stuart Marks wrote:
Hi Tristan,
Changes mostly look good.
There is an ignored InterruptedException in both versions of
UseCustomSocketFactory.java, but that was there already; it's not
clear what should be done in this case anyway. This is something to
keep in mind for a future cleanup. Hm, some duplicate code here as
well, again something to think about for the future.
There is a serious problem with the change to ReadTimeoutTest.java,
however. The change removes the (old) line 72, which is
TestIface stub = impl.export();
probably because there was an IDE warning that the variable "stub" is
unused. This much is true, but it's essential for impl.export() to be
called, because that exports the object, which creates a socket using
the socket factory, which eventually results in the fac.whichPort()
call below returning the port that was open. In the absence of the
export() call, whichPort() returns zero which causes the test to abort
immediately.
In addition, the refactoring to use try-with-resources changes the
order of execution of certain code, and it changes the scope of things
handled by the finally-block.
One purpose of the finally-block is to unexport the remote object so
it makes sense to begin the try-block immediately following the
export. The original code did this (well, only after a benign local
variable declaration). The change moves the try-block few lines down,
which means there is a larger window of time within which the
finally-block won't be executed. This isn't obviously a problem, but
it's a change nonetheless.
Also, the change alters the order of opening the client socket and the
"connecting to listening port" message, so the message comes after the
port is opened, instead of before. Again, an apparently small change,
but if there's an exception opening the port, the port number being
opened won't be printed out.
The main point of the changes to this file, however, is good, which is
to replace the unsafe use of multi-thread access to a boolean array
and polling of that value, with a CountDownLatch. So that part of the
change should go in. The problem is the apparently innocuous code
cleanups (use of try-with-resources, removal of apparently unused
local variable) which cause the test to break or otherwise change its
behavior.
I could go ahead and push this changeset, omitting the changes to
ReadTimeoutTest.java. Or, you could update the changeset to revert all
of the changes to ReadTimeoutTest.java except for those necessary to
implement the use of CountDownLatch. Either way is fine with me.
Which would you prefer?
s'marks
On 12/18/13 6:51 AM, Tristan Yan wrote:
Hi Everyone
Please review the code fix for bug JDK-7168267
http://cr.openjdk.java.net/~tyan/JDK-7168267/webrev.01/
<http://cr.openjdk.java.net/%7Etyan/JDK-7168267/webrev.01/>
This is a cleanup for RMI tests. trying to use real timeout to
replace a fixed number of loop.
Thank you
Tristan
On 12/12/2013 05:33 AM, Stuart Marks wrote:
On 12/10/13 6:10 PM, Tristan Yan wrote:
/Hi everyone
I am working on bug JDK-7168267
Correct link is
https://bugs.openjdk.java.net/browse/JDK-7168267
Root Cause:
- Per Stuart's comment, this is a clean up bug.
Suggested Fix:
- Will use timeout to replace loop.
We should probably look at specific cases for this. There are places
where the test is waiting for some external service to become ready
(e.g., rmiregistry). There's no notification for things like this so
wait-with-timeout cannot be used. Pretty much the only thing that
can be done is to poll reasonably often until the service is ready,
or until the timeout is exceeded.
- Also I am fixing two test's performance
java/rmi/activation/Activatable/forceLogSnapshot - method
waitAllStarted is
using sleep to poll 50 restartedObject to be true, we can use modern
CountDownLatch to implement blocking-time wait.
java/rmi/activation/Activatable/checkAnnotations - We can subclass
ByteArrayOutputStream which support notification when data was
written. Also use
two thread wait output string and error string to be not null.
These sound reasonble. Go ahead and file sub-tasks for these and
then choose one to work on first. (I think it will get too confusing
if we try to work on them all simultaneously.) Either post a
detailed description of what you intend to do, or if it's simple
enough, just post a webrev.
s'marks
Please let me know if you have any comments or suggestions.
/ /
Thank you
Tristan
On 12/05/2013 09:02 AM, Stuart Marks wrote:
/
/On 12/3/13 11:05 PM, Tristan Yan wrote:
/
/I am working on
https://bugs.openjdk.java.net/browse/JDK-7168267. This bug is
asking performance improvement for RMI test. Because this would
involve
different RMI tests. I’d like to use this cr as an umbrella bug,
create sub-cr
for different test. Then I can make progress on sub-cr. Please
let me know your
opinion on this.
/
/
Actually JDK-7168267 is more about various test cleanups, and
JDK-8005436 is
more about performance. Both bugs, though, make general statements
about "the
RMI tests" and don't have much information about specific actions
that need to
be taken. I've added some notes to JDK-7168267 about some cleanups
that could
be done.
/ /
If there are specific actions for either of these bugs, then yes,
creating
Sub-Tasks of these bugs and fixing them individually is the right
thing to do.
/ /
s'marks
/
/
/
