[OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI up-call made whilst holding JNI critical lock.
Jayathirth D V
jayathirth.d.v at oracle.com
Mon Sep 12 15:42:08 UTC 2016
Hi Prasanta,
Thanks for your review.
There is no need to RELEASE_ARRAYS() for GET_ARRAYS() in line number 578, 588, 994 or 1085.
All these calls are coming internally from IJG library and if we have to just make sure that we include these up calls in RELEASE_ARRAY- GET_ARRAY pair.
If everything goes right at the end of readImage(), readImageHeader(), writeImage() or writeImageHeader() we actually call RELEASE_ARRAY(), which will unpin these buffers which were acquired at the start of these functions.
Regarding "2856 RELEASE_ARRAYS(env, data, (const JOCTET *)(dest->next_output_byte));". We don't need this RELEASE_ARRAY() call at this place as we have not yet pinned any buffer. So I have removed it.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/8162461/webrev.02/
And I have verified jtreg, JCK and JPRT for all the changes.
Thanks,
Jay
-----Original Message-----
From: Prasanta Sadhukhan
Sent: Monday, September 12, 2016 2:05 PM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI up-call made whilst holding JNI critical lock.
Don't we need to call RELEASE_ARRAYS for these GET_ARRAYS 578 || !GET_ARRAYS(env, data, &(src->next_input_byte))) {
588 || !GET_ARRAYS(env, data, 994 || !GET_ARRAYS(env, data, &(src->next_input_byte))) {
1085 || !GET_ARRAYS(env, data, &(src->next_input_byte))) {
Also, we are calling
2856 RELEASE_ARRAYS(env, data, (const JOCTET *)(dest->next_output_byte));
before
2861 JNU_ThrowByName(env, "javax/imageio/IIOException", buffer);
but not before
2843 JNU_ThrowByName( env,
2844 "java/lang/OutOfMemoryError",
2845 "Writing JPEG Stream");
Shouldn't we need to call RELEASE_ARRAYS before line 2843 too?
I don't see any GET_ARRAYS here so probably 2856's RELEASE_ARRAYS is not needed!!
Lastly, I hope JPRT passes with this change.
Regards
Prasanta
On 9/9/2016 2:56 AM, Philip Race wrote:
> 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
>>>
More information about the 2d-dev
mailing list