[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in java.awt.image.FilteredImageSource.startProductio
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2017-11-29 13:56:22
Message-ID: 46347199-62e0-4ff0-19f8-de808cb335c3 () oracle ! com
[Download RAW message or body]

Looks fine, thank you.

On 29/11/2017 04:44, Prahalad Kumar Narayanan wrote:
> 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.
> 


-- 
Best regards, Sergey.


[prev in list] [next in list] [prev in thread] [next in thread]