<AWT Dev> [8] Review request 7059886: 6 JCK manual awt/Desktop tests fail with GTKLookAndFeel - GTK intialization issue

[Alexander Zvegintsev]

Hi Anthony, Artem,

here is a new version of webrev with suggested improvements:
http://cr.openjdk.java.net/~serb/alexz/7059886/webrev.01/

Name of a function in GThreadHelper is not ideal, but I can't think out 
any better.

> We don't expect C code to use try - finally, but when we use 
> GThreadHelper from FX, we'll do. 
In FX we will use it from native too.

> BTW, is g_thread_get_initialized() really deprecated? If that is the 
> case, it doesn't worth to use it, but rely on GThreadHelper only. 
I think it worth to use it. It is deprerecated due to:
> |g_thread_init| has been deprecated since version 2.32 and should not 
> be used in newly-written code. The GLib threading system is
> automatically initialized at the start of your program.
hence g_thread_get_initialized() returns always true on a modern GLib 
versions.


Thanks,

Alexander.

On 09/24/2013 04:12 PM, Anthony Petrov wrote:
> Hi Alexander,
>
> A few comments on the GThreadHelper class:
>
> 1. I think the static methods in the GThreadHelper class should be 
> public, because they are intended to be called by external code. Note 
> that all public symbols should have a javadoc.
>
> 2. Why do we need both isInitializationNeeded() and initFinished()? 
> Can the isInitializationNeeded() act (and be named) like the 
> AtomicBoolean.compareAndSet/getAndSet() methods, in that it would 
> automatically set the initialized flag to true?
> Or do you see a use case when a code may want to check that GTK isn't 
> initialized yet and still not initialize it afterwards? What could 
> this be needed for?
>
> 3. Since the flag has to be queried/modified under the LOCK only, it 
> doesn't need to be volatile.
>
> 4. In Javadocs, the first sentence usually summarizes what a method 
> does. Locking requirements should be stated either at the end of the 
> first sentence, or in a subsequent sentence.
>
> -- 
> best regards,
> Anthony
>
> On 09/24/2013 02:40 PM, Alexander Zvegintsev wrote:
>> Hello,
>>
>> Please review the fix for the issue:
>> https://bugs.openjdk.java.net/browse/JDK-7059886
>> The webrev is available here:
>> http://cr.openjdk.java.net/~serb/alexz/7059886/webrev.00/
>>
>> For old versions of GLib (< 2.24) calling g_thread_init () [1] multiple
>> times will crash an application.
>> There are two ways to find out if g_thread_init() has been called:
>> g_thread_supported ()[2] and g_thread_get_initialized ()[3]
>>
>> g_thread_supported () is a macro, so we cannot load it with dlsym
>> g_thread_get_initialized () was introduced in 2.20, but we have to
>> support versions < 2.20
>>
>> Currently in JDK we have an internal flag which protects us from such
>> multiple calls, but at least we
>> have Java FX which does not have access to this flag.
>>
>> The idea of the fix it to make single entry point for everyone who is
>> about to call g_thread_init () to
>> check if it has been called.
>>
>> Should we bring back g_thread_get_initialized () check for GLib >= 2.20
>> for safety?
>>
>> [1]
>> https://developer.gnome.org/glib/stable/glib-Deprecated-Thread-APIs.html#g-thread-init 
>>
>>
>> [2]
>> https://developer.gnome.org/glib/stable/glib-Deprecated-Thread-APIs.html#g-thread-supported 
>>
>>
>> [3]
>> https://developer.gnome.org/glib/stable/glib-Deprecated-Thread-APIs.html#g-thread-get-initialized 
>>
>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20130927/1e5f3b29/attachment-0001.html