On 30/08/2016 1:52 PM, Harsha Wardhana B wrote:
David,
Does accessing under synchronization lock guarantee visibility to
variable modifications?
Yes. As long as all accesses to the variable are with the
synchronization lock held then you are guaranteed to see all changes.
volatile is only needed for lock-free accesses.
David
-Harsha
On Tuesday 30 August 2016 08:21 AM, David Holmes wrote:
On 30/08/2016 9:08 AM, Daniel D. Daugherty wrote:
On 8/23/16 6:17 AM, Harsha Wardhana B wrote:
Hi David,
You approach to waiter object is a neat little abstraction and I will
make it a point to use it in future fixes, if required.
revised webrev : http://cr.openjdk.java.net/~hb/8152589/webrev.02/
test/java/lang/management/ThreadMXBean/Locks.java
General comment:
'synchronized(foo)' and 'synchronized (foo)' are both used.
Please pick a style and use it consistently.
L24: /*
Space added before comment block begin. Please remove it.
L76: /*
L77: Handy debug function to check if error condition is because
of test code or not.
L78: */
Should look like this:
L76: /*
L77: * Handy debug function to check if error condition is
because of test code or not.
L78: */
That is standard block comment format.
L114: /*
L115: Do slow check if thread is blocked on a lock. It is
possible that last thread
L116: to come out of Phaser might still be in Phaser call stack
(Unsfe.park) and
L117: hence might eventually acquire expected lock.
L118: */
Same comment about standard block comment format.
Also typo: "Unsfe.park" -> "Unsafe.park".
L129: while(p.test(result)){
Space before first '(' and before '{'.
L171: // stop here for LockBThread to hold OBJB
Perhaps this for the comment:
// block here while LockBThread holds OBJB
L208: /*
L209: Must be invoked from within a Synchronized context
L210: */
Same comment about standard block comment format.
Also not sure why "Synchronized" is capitalized...
L213: boolean isNotified = false;
This should probably be volatile.
Update: WaitingThread calls OBJC.doWait() and
CheckerThread calls OBJC.doNotify(). Yup, it should
be volatile.
Nope - it is only accessed under synchronization lock, which must be
held by the caller else the wait()/notify() will throw
IllegalMonitorStateException.
David
-----
Dan
On Tuesday 23 August 2016 11:47 AM, David Holmes wrote:
Hi Harsha,
On 22/08/2016 6:48 PM, Harsha Wardhana B wrote:
Hello,
Please review the below webrev incorporating David's comments.
http://cr.openjdk.java.net/~hb/8152589/webrev.01/
Using a static isNotified field isn't exactly what I had in mind, I
was thinking of something more encapsulated - something I thought
already existed in other tests, but now I can't find it. Eg:
/* synchronization helper for two-thread wait/notify interaction
*/
static class WaitObject {
boolean isNotified = false;
public void await() throws InterruptedException {
while (!isNotified)
wait();
isNotified = false;
}
public void doNotify() {
isNotified = true;
notify();
}
}
then OBJC would be a WaitObject and you would do:
synchronized(OBJC) {
log("WaitingThread about to wait on objC");
try {
// Signal checker thread, about to wait on objC.
waiting = false;
p.arriveAndAwaitAdvance(); // Phase 1 (waiting)
waiting = true;
OBJC.doWait();
} catch (InterruptedException e) {
...
etc.
Minor nits:
- why did you move the @library ?
It was suggested by NetBeans Jtreg plugin to fix tag order.
- @Override, if used, should be applied consistently
Have applied the annotation consistently.
- if you want to capitalize objA, objB, objC then ensure you also
update it in the comments and log statements (it really isn't
necessary to capitalize them, that is suggested for compile-time
constants and these are not - they are just static final variables).
Done.
Thanks
Harsha
Thanks,
David
Regards
Harsha
On Wednesday 17 August 2016 11:45 AM, Harsha Wardhana B wrote:
Hi David,
I will incorporate changes suggested by you. Let's wait for few more
review comments and then I will send consolidated webrev.
Regards
Harsha
On Wednesday 17 August 2016 09:02 AM, David Holmes wrote:
On 16/08/2016 11:33 PM, Harsha Wardhana B wrote:
Hi David,
Agreed that we could fix WaitingThread the way you have said,
but in
recent past, there aren't any issues reported w.r.t WaitingThread.
Nor are there likely to be - that's what makes spurious wakeup bugs
so difficult to detect!
This test has been fixed several times (3-4) for intermittent
failures
and hence I would not like to meddle with code that is not
causing any
problems even though there is scope for refactoring.
It isn't refactoring it is fixing and we have fixed several
tests in
a similar way.
The issue reported was with LockThreadB and hence I have provided
possible fix for the same.
That doesn't preclude fixing other issues with the test at the same
time.
David
Thanks
Harsha
On Tuesday 16 August 2016 01:32 PM, David Holmes wrote:
Hi Harsha,
On 16/08/2016 4:08 PM, Harsha Wardhana B wrote:
Hello,
Please review and provide comments for fix for issue,
https://bugs.openjdk.java.net/browse/JDK-8152589
having webrev located at
http://cr.openjdk.java.net/~hb/8152589/webrev.00/
These changes look quite good (though I have to admit I
struggle to
read the lambda and stream code :) ).
Note that like many of these kinds of tests there is an issue
with
WaitingThread because it does not wait in a loop and so is
susceptible
to spurious wakeups. The way to fix that is to add a "notified"
variable and then do:
while (!notified)
wait();
and set notified before the notify().
Thanks,
David
Fix details:
1. From nightly failures we see that LockThreadB was blocked on
wrong
object. We now do a repeated check with timeout if any given
thread is
blocked on expected object. It is possible that LockThreadB
might
still
be in Phaser call stack (Unsafe.park) when
'checkBlockedObject' is
invoked.
2. The logs from lock free logger was never printed. It is not
being
printed.
3. Any time we see failure, thread stack is being logged. This
helps us
ascertain if failure is in test case or in the component.
4. Even though we had lock free logger, several
ex.printStackTrace() was
used which could be responsible for failure. It is removed.
5. There were several places where tests continue to ran even
after
failure (testFailed flag). That is fixed.
6. Couple of other minor refactoring.
Thanks
Harsha
