I think the v.1 will be better even if it is not showing up as an issue,
given that you seem to have a safe + simple fix to that part
So "+1" to "1"
-phil.
On 9/8/16, 8:09 AM, Jayathirth D V wrote:
Hi Phil,
More observations:
All emit_message() calls come under macros defined in jerror.h like WARNXX and
TRACEXX.
I made changes in IJG library so that we call these WARNXX and TRACEXX macros
forcefully in turn calling emit_message()(emit_message() also changed to call
output_message() everytime).
With the above change and without RELEASE/GET_ARRAYS change in
sun_jpeg_output_message() also JVM is not throwing any error. It means when we
enter sun_jpeg_output_message() we don't have any active JNI critical lock.
We can actually use http://cr.openjdk.java.net/~jdv/8162461/webrev.00/ without
RELEASE/GET_ARRAYS change in sun_jpeg_output_message() or we can use
http://cr.openjdk.java.net/~jdv/8162461/webrev.01/ having RELEASE/GET_ARRAYS
change in sun_jpeg_output_message() for future proofing.
Thanks,
Jay
-----Original Message-----
From: Jayathirth D V
Sent: Thursday, September 08, 2016 7:21 PM
To: Philip Race; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI up-call made
whilst holding JNI critical lock.
Hi Phil,
Thanks for pointing me to sun_jpeg_output_message().
We need to make up calls from sun_jpeg_output_message() as it is needed if
user has added IIOReadWarningListener.
In imageioJPEG.c we are overriding IJG output_message() of jerror.c with our
own sun_jpeg_output_message().
Call from IJG library can reach output_message() through two functions in
jerror.c :
1) error_exit()
2) emit_message()
We are overriding error_exit() also with sun_jpeg_error_exit() where we are not
calling output_message(), so sun_jpeg_output_message() can be reached only
through emit_message() .
emit_message() always takes j_common_ptr as argument and not j_compress_ptr or
j_decompress_ptr. But I noticed that before calling emit_message() we are
always type casting j_compress_ptr or j_decompress_ptr to j_common_ptr and
passing it as argument to emit_message().
Since cinfo-> is_decompressor tells us whether it is read or write operation
we can cast j_common_ptr to j_compress_ptr or j_decompress_ptr. Based on this I am
creating jpeg_source_mgr or jpeg_destination_mgr object. Using this we can call
RELEASE/GET_ARRAYS in sun_jpeg_output_message().
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8162461/webrev.01/
Parallel to this I tried using two separate functions like
sun_jpeg_reader_output_message(j_decompress_ptr) and
sun_jpeg_writer_output_message(j_compress_ptr). But then we need to replicate
error_exit() and emit_message() also to accept j_decompress_ptr and
j_compress_ptr. It needs lot of changes at many places where we are using
error_exit() and emit_message() in IJG library.
Thanks,
Jay
-----Original Message-----
From: Phil Race
Sent: Wednesday, September 07, 2016 10:57 PM
To: Jayathirth D V; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI up-call made
whilst holding JNI critical lock.
This all looks reasonable. But I wonder if you missed something.
Take a look at sun_jpeg_output_message(). That may also make up-calls.
A pointer to this function is passed to the IJG library and I don't know the
circumstances under which it may call this function.
If it ever might do it whilst we are holding these locks then that would mean
we'd need the same RELEASE/GET in there .. although the challenge would be that
it does not have access to the data to release the arrays.
So
1) Investigate if it is an actual issue,
2) If it is, then decide between a way to ensure the arrays are available to
this function (looks tricky to me), or somehow deferring these up-calls or
eliminating them.
-phil.
On 9/6/2016 11:49 PM, Jayathirth D V wrote:
Fixed typo.
*From:* Jayathirth D V
*Sent:* Wednesday, September 07, 2016 12:11 PM
*To:* Philip Race; 2d-dev
*Subject:* [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI
up-call made whilst holding JNI critical lock.
Hi,
Please review the following fix in JDK9 at your convenience:
Bug : https://bugs.openjdk.java.net/browse/JDK-8162461
Webrev : http://cr.openjdk.java.net/~jdv/8162461/webrev.00/
<http://cr.openjdk.java.net/%7Ejdv/8162461/webrev.00/>
Issue : If we try to perform operations like
reader.abort()/reader.dispose()/ reader.reset() in any of the
IIOReadUpdateListener callbacks, JVM will throw deadlock error.
Root cause : We are making callbacks from native side to Java by
holding some resources in JNI critical lock.
Solution : We have to release the JNI critical lock on the resources
before we call Java function from native side. If we have JNI critical
lock and we throw an Exception in that case also we should release the
lock.
Thanks,
Jay
