Hi Prasanta,
Actually returns null in test description is for
createImageXXXStream() and not for read() and write() functions. But
for more clarity I have added why we catch IOException also in test
description.
Please review the updated webrev:
http://cr.openjdk.java.net/~jdv/8044289/webrev.05/
<http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.05/>
Thanks,
Jay
*From:*prasanta sadhukhan
*Sent:* Tuesday, March 29, 2016 2:21 PM
*To:* Jayathirth D V
*Cc:* Philip Race; 2d-dev@openjdk.java.net
*Subject:* Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In
ImageIO.read() and write() NPE is not handled properly for stream
+1.
Looks ok to me. Only thing is that whether the test summary is
appropriate "
Test verifies that when createImageInputStream() or
28 * createImageOutputStream() returns null while read or write,"
Should it not be "verifies whether ...throws appropriate exception"?
Regards
Prasanta
On 3/29/2016 3:42 AM, Philip Race wrote:
This all looks fine to me. Although no new Exception types are thrown
I think we need a CCC anyway, so please prepare and get that approved
before pushing.
-phil.
On 3/28/16, 3:30 AM, Jayathirth D V wrote:
Hi Phil,
After discussion with Sergey we have come to conclusion that
Sergey is okay with throwing exception or returning null/false.
So I have modified the webrev to throw exception.
Also in write() I have modified the catch block to just throw
the caught exception without adding message since it will
create confusion between the case where we are not able to
create cache and when createImageOutputStream() returns null.
Regarding catch block of createImageOutputStream() only when
we usecache is true we use FileCacheImageOutputStream() and it
throws IOException so I think there is no need to add extra
check before throwing IOException.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/8044289/webrev.04/
<http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.04/>
Thanks,
Jay
*From:*Jayathirth D V
*Sent:* Thursday, March 24, 2016 3:52 PM
*To:* Philip Race
*Cc:* Sergey Bylokhov; 2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>
*Subject:* RE: [OpenJDK 2D-Dev] Review Request for JDK-8044289
: In ImageIO.read() and write() NPE is not handled properly
for stream
Hi Phil,
We can change the code to something like :
1522 * @return <code>false</code> if no appropriate
writer is found or
1523 * when needed stream is null.
1545 try {
1546 output.delete();
1547 stream = createImageOutputStream(output);
1548 } catch (IOException e) {
1549 *throw e;*
1550 }
In this case we will be throwing IOException when we are not
able to create cache file and stream is null.
And return null/false when createImageXXXStream returns null.
Also we can add check before throwing IIOException from
createImageXXXStream for usecache.
350
351 boolean usecache = getUseCache() &&
hasCachePermission();
352
353 while (iter.hasNext()) {
354 ImageInputStreamSpi spi = iter.next();
355 if (spi.getInputClass().isInstance(input)) {
356 try {
357 return
spi.createInputStreamInstance(input,
358
usecache,
359
getCacheDirectory());
360 } catch (IOException e) {
361 *if (usecache)*
362 throw new IIOException("Can't
create cache file!", e);
363 }
364 }
365 }
This will be one of the approach.
Or we can throw IOException in all the cases with additional
check in createImageXXXStream for usecache. This will be like
throwing IOException when createImageXXXStream returns
null/false or it throws IIOException when it cant create cache
file. This will be another approach. Please let us know your
inputs.
Thanks,
Jay
*From:*Philip Race
*Sent:* Thursday, March 24, 2016 5:10 AM
*To:* Jayathirth D V
*Cc:* Sergey Bylokhov; 2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>
*Subject:* Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289
: In ImageIO.read() and write() NPE is not handled properly
for stream
I don't think this is ready and we need to discuss whether to
rework it.
In general I think I prefer IIO rather than null when we have a stream
problem.
For one thing you have this change :-
1522 * @return <code>false</code> if no appropriate
writer is found or
1523 * not able to create required ImageOutputStream.
yet the implementation says :-
1545 try {
1546 output.delete();
1547 stream = createImageOutputStream(output);
1548 } catch (IOException e) {
1549 throw new IIOException("Can't create output stream!",
e);
1550 }
So whilst "null" would mean we cannot find an IOS SPI it isn't
the only reason we fail to create the IOS
A null return seems to get passed through to doWrite() which probably
throws IIOE
And I think that the implementation is actually right here where
it throws an exception.
The ability to create an image input or output stream could be because
of some kind of problem opening the file, or with the IIS or IOS
implementations -
like they were de-registered. I think in many if not all of these cases
IOE - or IIOE is
what we want.
Also since we call createImageInputStream() - which is public - we
need to look at that, and what it does, which is also something to
consider.
If it returns null, that seems to mean that no suitable SPI is
registered
which can only happen if they are de-registered. For the methods where
we use createImageInputStream() I think it is fair to turn that into
IIOE
BTW I note that the code there says :-
350
351 boolean usecache = getUseCache() && hasCachePermission();
352
353 while (iter.hasNext()) {
354 ImageInputStreamSpi spi = iter.next();
355 if (spi.getInputClass().isInstance(input)) {
356 try {
357 return spi.createInputStreamInstance(input,
358 usecache,
359
getCacheDirectory());
360 } catch (IOException e) {
361 throw new IIOException("Can't create cache
file!", e);
362 }
363 }
364 }
So far I can see that means it will claim any Exception that is
generated is because it
could not create the cache file without any proof of that and even if
"useCache" is false.
It seems to imply to me that the author was not considering things like
someone
passing a closed InputStream .. probably we ought to be more vague in
this case
or look more deeply at what we can tell from whether it is IIOE or IOE.
-phil.
On 3/22/16, 10:49 AM, Jayathirth D V wrote:
Thanks for the review Sergey.
Can I get +1 for this please?
-Jay
-----Original Message-----
From: Sergey Bylokhov
Sent: Tuesday, March 22, 2016 9:52 PM
To: Jayathirth D V; Philip Race
Cc:2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In
ImageIO.read() and write() NPE is not handled properly for stream
This fix looks fine to me.
At least it made all code work in a similar way. But probably
someone will have other opinion?
On 22.03.16 12:34, Jayathirth D V wrote:
Hi Sergey,
I have unified changes related to ImageIO.read() and
ImageIO.write().
In case of read() we will be returning null when
createImageInputStream() returns null.
In case of write() we will be returning false when
createImageOutputStream() returns null.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8044289/webrev.03/
<http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.03/>
Thanks,
Jay
-----Original Message-----
From: Sergey Bylokhov
Sent: Tuesday, March 22, 2016 7:39 AM
To: Jayathirth D V; Philip Race
Cc:2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 :
In
ImageIO.read() and write() NPE is not handled properly for
stream
Hi, Jay.
Since we decided to update the specification we get an additional
flexibility in the fix, because we can update the "@exception IOException" part
of javadoc and describe that it can be thrown if ImageXXXStream cannot be created.
I don't see a big differences of both approaches in case of
write() methods, but in case of read(which has the similar issue but was
skipped in the fix) there is a difference.
Note that the read(File) unlike other methods throws the
unspecified
IIOException exception if ImageXXXStream cannot be created. We
can
think about unification of read/write methods in ImageIO. Hmm
but even
then
read() in some cases will return null and in some cases throw
an exception....
and createImageInputStream/createImageOutputStream() will look
alien as they always return null not an exception.
Another possible unification is to update javadoc for all
methods,
that they will return null if related providers are not
found(in this
case "throw new IIOException("Can't create an
ImageInputStream!"))"
should be removed from read(File);
On 21.03.16 10:26, Jayathirth D V wrote:
Hi Sergey,
For the second approach I have created webrev for review.
Please review updated webrev :
http://cr.openjdk.java.net/~jdv/8044289/webrev.02/
<http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.02/>
Thanks,
Jay
-----Original Message-----
From: Jayathirth D V
Sent: Friday, March 18, 2016 2:23 PM
To: Sergey Bylokhov
Cc:2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>; Philip Race
Subject: RE: [OpenJDK 2D-Dev] Review Request for
JDK-8044289 : In
ImageIO.read() and write() NPE is not handled properly for
stream
Hi Sergey,
Thanks for your inputs.
Indeed all the null values returned by
createImageInputStream() and createImageInputStream() are valid only.
I went through the code more and found that there is no
need for fix in ImageIO.read() methods as null stream is handled in
read(ImageInputStream stream) which is called by all the remaining read() API's
through IAE. So i can remove the changes made in ImageIO.read() functions.
Also istream is closed in finally block of read(url).
For ImageIO.write(File) & ImageIO.write(OutputStream) NPE
we have two approaches for fix:
1) Keep the present changes of throwing IOException when
stream is null and change the throw clause of write() spec mentioning:
"IOException - if an error occurs during writing or not
able to create required ImageOutputStream"
2) Remove the present changes and handle null in doWrite()
method by returning false when stream is null & add code in finally block to
close the stream only when stream is not null. Along with this we can also
change Returns clause of spec to mention that write() might return false when we
are not able to create ImageOutputStream.
private static boolean doWrite(RenderedImage im,
ImageWriter writer,
ImageOutputStream output) throws IOException {
if (writer == null) {
return false;
}
+ if (output == null) {
+ return false;
+ }
try {
return doWrite(im, writer,
stream);
} finally {
+ if (stream != null)
stream.close();
}
Returns :
false if no appropriate writer is found or
not able to create required ImageOutputStream.
Please let me know your inputs.
Thanks,
Jay
-----Original Message-----
From: Sergey Bylokhov
Sent: Friday, March 18, 2016 12:24 AM
To: Jayathirth D V
Cc:2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>; Philip Race
Subject: Re: [OpenJDK 2D-Dev] Review Request for
JDK-8044289 : In
ImageIO.read() and write() NPE is not handled properly for
stream
On 17.03.16 21:14, Jayathirth D V wrote:
There are many places in createImageInputStream() and
createImageOutputStream() where it will return null for stream.
But when why you change the write/read methods? They should
take null into account and work according to specification. If some bogus null
returns from createImageInputStream/createImageOutputStream then it should be
fixed in these methods.
So for NPE described in bug to occur there can be
multiple paths:
1) One of the path described in bug is when
theRegistry.getServiceProviders() throws IAE and we return null.
This is the case when we have no installed spi and should
return null/false.
2) When we pass non-existent file path to ImageIO (as
described
inhttp://stackoverflow.com/questions/11153200/with-imageio-write-api-call-i-get-nullpointerexception
). Here we catch FileNotFoundException but still we return stream as null at
the end which will cause NPE further.
This is same thing described in the bug(about
stream.close()). The write method call createImageOutputStream() and got
FileNotFoundException.
This exception should be propagated to the user, but it is
replaced in the finally block, because we try to close the stream=null. So the
fix for this particular case is to check the stream to null in finally, No?
3) When we don't support the provider and
theRegistry.getServiceProviders() return iterator with size zero.
This means that our providers do not support this case, and
we should return null/false.
For us to follow first scenario we need access to
categoryMap of IIORegistry and delete the ImageInputStreamSpi/
ImageOutputStreamSpi but I was not able to find a way to tamper it from test
case. If we follow second case FileNotFoundException will be thrown by default
eventhough we handle NPE.
But you change the code and we now contradicts the
specification, in your case the test should expect false/null from
write()/read().
In the test case we are trying to following third
scenario to get stream as null to verify the code changes.
To catch the bogus null we should try to use another
approach, or change the spec. But it seems all null values above are correct,
we should be ready for them.
As part of collective fix we are throwing IOException
which will indicate that we are not able to create required stream.
Thanks,
Jay
-----Original Message-----
From: Sergey Bylokhov
Sent: Thursday, March 17, 2016 7:27 PM
To: Phil Race
Cc: Jayathirth D V;2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] Review Request for
JDK-8044289 : In
ImageIO.read() and write() NPE is not handled properly
for stream
On 17.03.16 0:36, Phil Race wrote:
I don't think that is the actual reason here
But the test doing exactly this thing -> deregister all
spi => in
this case according to specification all related
methods(read/write/creatImageInputStream/creatImageOutputStream) should return
null or false.
- it seems likely that there
was some kind of internal error or bad SPI causing
this.
So throwing an exception seems more appropriate.
the problem was in some of our methods which do not
expect null in some places(like stream.close() from the bug description).
And also the bug was originally solely about
write(..).
-phil.
On 03/16/2016 01:35 PM, Sergey Bylokhov wrote:
As far as I understand the
createImageInputStream() returns null
is this stream is unsupported by current
plugins via spi. Probably
the
read() method should return null too?
/**
* Returns a {@code BufferedImage} as
the result of decoding
* a supplied {@code InputStream} with
an {@code ImageReader}
* chosen automatically from among
those currently registered.
* The {@code InputStream} is wrapped
in an
* {@code ImageInputStream}. If no
registered
* {@code ImageReader} claims to be
able to read the
* resulting stream, {@code null} is
returned.
*
.......
* @return a {@code BufferedImage}
containing the decoded
* contents of the input, or {@code
null}.
*/
public static BufferedImage
read(InputStream input) throws
IOException
On 16.03.16 20:29, Philip Race wrote:
The test writes out into "test.src".
I believe that you should treat that as a
"read-only" location.
Write to a tempfile (File.createTempFile())
or TESTCLASSES.
-phil.
On 3/15/16, 10:50 PM, Jayathirth D V wrote:
Hi Phil,All
_Please review the following fix in
JDK9:_
__
Bug
:https://bugs.openjdk.java.net/browse/JDK-8044289
Webrev
:http://cr.openjdk.java.net/~jdv/8044289/webrev.00/
<http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/>
<http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/>
<http://cr.openjdk.java.net/%7Ejdv/8044289/webrev.00/>
Issue : When createImageInputStream()
or createImageOutputStream
returns null in ImageIO.read() or
ImageIO.write() we are seeing
NPE as it stream is used further
without null check.
Root cause : "stream" is used without
checking whether it is
null or not.
Solution : Handle null pointer of
stream and throw IIOException.
Test case contains all possible
scenarios where
createImageInputStream() or
createImageOutputStream can return null.
Thanks,
Jay
--
Best regards, Sergey.
--
Best regards, Sergey.
--
Best regards, Sergey.
--
Best regards, Sergey.
