Hello Everyone
First, Thanks to Sergey for his detailed suggestion on test-case approach.
I've modified the test-case keeping Sergey's suggestion as a reference.
Few subtle changes are-
. Created explicit classes rather than using anonymous class definitions
. Modified minimum test duration from 10 seconds to 5 seconds.
The test-case now reproduces this issue without the proposed fix. (Tested on
win7 and Ubuntu VM)
Besides this, I 've updated Javadoc comments for FilteredImageSource as
requested by Joe Darcy in CSR review.
. Since every method is synchronized, I decided to update javadoc comments
for the class.
. The approach is similar to how java.util.HashTable captures the
synchronized contract of its methods.
The javadoc modification is as follows-
@@ -35,7 +35,8 @@
/**
* This class is an implementation of the ImageProducer interface which
* takes an existing image and a filter object and uses them to produce
- * image data for a new filtered version of the original image.
+ * image data for a new filtered version of the original image. Furthermore,
+ * {@code FilteredImageSource} is synchronized for thread-safety.
* Here is an example which filters an image by swapping the red and
Kindly review the above changes at your convenience and share your views
Webrev link: http://cr.openjdk.java.net/~pnarayanan/8188083/webrev.02/
Thank you for your time
Have a good day
Prahalad
-----Original Message-----
From: Sergey Bylokhov
Sent: Tuesday, November 28, 2017 11:14 PM
To: Prahalad Kumar Narayanan; 2d-dev
Cc: Philip Race
Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in
java.awt.image.FilteredImageSource.startProduction
The test can use any part of our API. It is a small part of the code which can
confirm/verify the fix. It should not be considered as a user's code and it
could emulate behavior of classes inside jdk.
If you do not like to call these methods directly you can do this like this:
import java.awt.Graphics;
import java.awt.Image;
import java.awt.image.ColorModel;
import java.awt.image.FilteredImageSource;
import java.awt.image.ImageConsumer;
import java.awt.image.ImageFilter;
import java.awt.image.ImageObserver;
import java.awt.image.ImageProducer;
import java.util.Hashtable;
public final class Test {
private volatile static Throwable fail;
public static void main(final String[] args) throws
InterruptedException {
final ImageConsumer ic = new ImageConsumer() {
@Override
public void setDimensions(int i, int i1) {
}
@Override
public void setProperties(Hashtable<?, ?> hashtable) {
}
@Override
public void setColorModel(ColorModel colorModel) {
}
@Override
public void setHints(int i) {
}
@Override
public void setPixels(int i, int i1, int i2, int i3,
ColorModel colorModel, byte[] bytes,
int i4,
int i5) {
}
@Override
public void setPixels(int i, int i1, int i2, int i3,
ColorModel colorModel, int[] ints,
int i4,
int i5) {
}
@Override
public void imageComplete(int i) {
}
};
final ImageProducer ip = new ImageProducer() {
@Override
public void addConsumer(ImageConsumer imageConsumer) {
}
@Override
public boolean isConsumer(ImageConsumer imageConsumer) {
return false;
}
@Override
public void removeConsumer(ImageConsumer imageConsumer) {
}
@Override
public void startProduction(ImageConsumer imageConsumer) {
}
@Override
public void requestTopDownLeftRightResend(
ImageConsumer imageConsumer) {
}
};
final Image image = new Image() {
ImageFilter filter = new ImageFilter();
ImageProducer producer = new FilteredImageSource(ip, filter);
@Override
public int getWidth(ImageObserver observer) {
return 100;
}
@Override
public int getHeight(ImageObserver observer) {
return 100;
}
@Override
public ImageProducer getSource() {
return producer;
}
@Override
public Graphics getGraphics() {
throw new UnsupportedOperationException();
}
@Override
public Object getProperty(String name, ImageObserver
observer) {
return null;
}
};
Thread t1 = new Thread(() -> {
try {
while (true) {
image.getSource().addConsumer(ic);
}
} catch (Throwable t) {
fail = t;
}
});
Thread t2 = new Thread(() -> {
try {
while (true) {
image.getSource().removeConsumer(ic);
}
} catch (Throwable t) {
fail = t;
}
});
Thread t3 = new Thread(() -> {
try {
while (true) {
image.getSource().startProduction(ic);
}
} catch (Throwable t) {
fail = t;
}
});
t1.setDaemon(true);
t2.setDaemon(true);
t3.setDaemon(true);
t1.start();
t2.start();
t3.start();
t1.join(10000);
if (fail != null) {
throw new RuntimeException("Test fail", fail);
}
}
}
On 27/11/2017 23:31, Prahalad Kumar Narayanan wrote:
> Hello Sergey
>
> Thank you for your review response.
>
> I had one version of the test that resembled your test code.
> I uploaded the result (screenshot) of that test case with/ without fix on JBS.
>
> The only point of concern - API documentation mentions that the methods of
> FilteredImageSource are not supposed to be invoked directly from user code.
> If the methods are invoked directly from user code, the result is un-defined.
> Since our JTreg test cases cannot violate the documented guide, I refrained
> from attaching the test case in the first webrev.
>
> Kindly let me know your views to this.
>
> Thank you once again
> Have a good day
>
> Prahalad
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Tuesday, November 28, 2017 12:46 PM
> To: Prahalad Kumar Narayanan; 2d-dev
> Cc: Philip Race
> Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in
> java.awt.image.FilteredImageSource.startProduction
>
> On 27/11/2017 22:12, Prahalad Kumar Narayanan wrote:
>> Yes..! As you rightly pointed, the test passes before the fix as well.
>
> Then I suggest to additionally reuse a test which I sent previously,
> it is fail almost always before the fix. The test which fails one time
> in
> 10000 runs will be never fixed, or such bugs will be closed as not
> reproducible.
>
>>
>> Reason is that, the occurrence of this bug is rare.
>> . Submitter has observed only 15 occurrences of the exception with
>> over 10,000 instances per month.
>> . Though the occurrence is rare, submitter is expecting a fix for this
>> issue.
>>
>> So what does this test-case resolve ?
>> . Our change adds "synchronized" contract to the method to fix the
>> Null pointer exception.
>> . To test the changes, we should check for both- Null Pointer
>> Exception and any deadlock that could arise from "synchronized" contract.
>> . The test case helps to address both but in a passive way. Why do I
>> say passive ?
>> . First, the test provides a hope to detect & report NPE though
>> its occurrence is rare.
>> . Second, any deadlock arising from the concerned method, will
>> cause the test to fail upon time-out.
>>
>> Thank you once again for your time
>> Have a good day
>>
>> Prahalad
>>
>> -----Original Message-----
>> From: Sergey Bylokhov
>> Sent: Monday, November 27, 2017 9:11 PM
>> To: Prahalad Kumar Narayanan; 2d-dev
>> Cc: Philip Race
>> Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in
>> java.awt.image.FilteredImageSource.startProduction
>>
>> Hi,Prahalad.
>> On 24/11/2017 21:40, Prahalad Kumar Narayanan wrote:
>>> Based on discussions with Sergey, I 've now updated the fix with a test
>>> case.
>>> The changes are available for review under:
>>> http://cr.openjdk.java.net/~pnarayanan/8188083/webrev.01/
>>
>> This version of the test is always passed before the fix(I have checked w/
>> and w/o jtreg).
>>
>> --
>> Best regards, Sergey.
>>
>
>
--
Best regards, Sergey.
