dev@javaserverfaces.java.net

Call for code review for BugDB 19859648

From: zhijun Ren <ren.zhijun_at_oracle.com>
Date: Fri, 24 Oct 2014 12:31:00 +0800

Hi Manfred and Ed,

Changed bundle text attached here, please help to review.

I plan to commit this fix to 2.2.9, is that right for this wls2.2.1 bug?

BR,
Zhijun

On 10/24/14, 10:37, zhijun Ren wrote:
> Thanks Manfred, I will send out the review quest today.
>
> On 10/23/14, 22:53, manfred riem wrote:
>> Hi Zhijun,
>>
>> OK go ahead and do that change and please run all the tests
>>
>> Thanks!
>> Manfred
>>
>> On 10/22/14, 10:21 PM, zhijun Ren wrote:
>>> Hi Manfred and Ed,
>>>
>>> The WeakHashMap acts as a cache for the MetaDataTarget here and it
>>> is static so there is one instance under one classloader. The
>>> concurrency accessing wasn't considered when the class was designed.
>>>
>>> Race condition comes in the MT environment, but it impacts not only
>>> this class but also the whole JSF framework, so we need to consider
>>> the integration of MT and JSF as a whole picture in terms of
>>> concurrency.
>>>
>>> So I think, changing the WeakHashMap by wrapping with synchronized
>>> makes sense for this bug in the current view, do you agree?
>>>
>>> BR,
>>> Zhijun
>>>
>>> On 10/22/14, 22:00, manfred riem wrote:
>>>> Hi Zhijun,
>>>>
>>>> We need to understand what it is that this particular map is having
>>>> the static WeakHashMap and if it might need to be replaced by a
>>>> non-static variant of this. What is this map used for?
>>>>
>>>> Thanks!
>>>> Manfred
>>>>
>>>> On 10/21/14, 9:57 PM, zhijun Ren wrote:
>>>>> Hi ED and Manfred,
>>>>>
>>>>> This bug's root cause is that in class
>>>>> com.sun.faces.facelets.tag.MetaRulesetImpl, its class
>>>>> variable(static), an instance of WeakHashmap, is accessed by
>>>>> multiple threads in the case of MT and it is not synchronized.
>>>>>
>>>>> My question is:
>>>>>
>>>>> 1. In MT case, should it be accessed by multiple threads?
>>>>>
>>>>> 2. If yes, the variable should be a synchronized map; otherwise,
>>>>> the class should be used in different naming spaces(under
>>>>> different classloader), right?
>>>>>
>>>>> BR,
>>>>> Zhijun