[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI up-call made whilst holding JNI critical l
From: Prasanta Sadhukhan <prasanta.sadhukhan () oracle ! com>
Date: 2016-09-13 7:13:23
Message-ID: 57D7A443.3040501 () oracle ! com
[Download RAW message or body]
Looks ok to me.
Regards
Prasanta
On 9/12/2016 9:12 PM, Jayathirth D V wrote:
> 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
> > > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic