Looks good to me Chris!
best regards,
-- daniel
On 19/11/2018 03:03, Chris Yin wrote:
Hi, Daniel
On 16 Nov 2018, at 7:13 PM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
Hi Chris,
This looks good to me, though I wonder if the check
for reason.length() > 0 should be made in
51 public synchronized String failureReason() {
52 return reason.length() > 0 ? reason.toString() : null;
53 }
instead of within passed() just to be on the safe side, in case
some subclass is tricked into thinking that there is a failure
if failureReason() starts returning "" instead of null?
Thanks for your suggestion, yes, that’s make sense since the original logic
“return reason” may also return null if no failure message, update webrev as
below.
http://cr.openjdk.java.net/~xyin/8129310/webrev.03/
Thanks,
Chris
best regards,
-- daniel
On 16/11/2018 03:58, Chris Yin wrote:
Sorry, one more revision here since I found we could do a little more in
subclasses (use loopback address to avoid possible outside noise), and this
will introduce the test class which extends AsyncCloseTest into webrev, hope
that will be convenient for review, thanks
http://cr.openjdk.java.net/~xyin/8129310/webrev.02/
Regards,
Chris
On 16 Nov 2018, at 10:39 AM, Chris Yin <xu.y....@oracle.com> wrote:
Hi, Daniel
Thanks a lot for your reviewing and comments, revision webrev as below.
http://cr.openjdk.java.net/~xyin/8129310/webrev.01/
Regards,
Chris
On 15 Nov 2018, at 6:06 PM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
Hi chris,
There's no reason to make `passed` and `closed` volatile since
they are private and only accessed from withing synchronized
methods.
On the other hand, reason could now be final.
Otherwise I guess it looks good. I haven't looked at the tests that
use AsyncClose so I trust that you have verified that your changes
work as you expect.
best regards,
-- daniel
On 15/11/2018 09:12, Chris Yin wrote:
Please review below minor change for java/net/Socket/asyncClose/AsyncClose.java
fails intermittently issue, thanks
From previous failure log, seems lack of enough info to find out root cause,
this change is to enhance a little to base test class AsyncCloseTest with
guessing, even it may not completely address the issue, additional log info
should still helpful for future debugging.
1. add volatile to 'boolean passed' and 'boolean closed' to avoid any
visibility issue since they were write/read in different threads
2. failure reason info may be overwritten if failed() been called multiple
times, change it to record all failure messages.
bug: https://bugs.openjdk.java.net/browse/JDK-8129310
webrev: http://cr.openjdk.java.net/~xyin/8129310/webrev.00/
Regards,
Chris
