[
https://issues.apache.org/jira/browse/MYFACES-3705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13636058#comment-13636058
]
Leonardo Uribe commented on MYFACES-3705:
-----------------------------------------
I have been thinking about this for a long time. Really the code is ok, use a
"copy on write" strategy works, but the evidence suggest that a
ConcurrentHashMap in that location will work too. There will not be any
difference from performance perspective, because the time spent compiling a
facelet or building a view or rendering a view is many times more than the lock
introduced by the map. Also the lock only happens when a facelet is added, so
once the facelet is compiled it will not be called anymore.
To be clear, the current code does not have any bug and it can be let as is.
But there is a problem with FaceletCache. MyFaces uses a special "composite
component metadata facelet" that the reference implementation does not have.
This means it is not possible to override fully how MyFaces handle facelet
instances, because there will be a part that is still done in
DefaultFaceletFactory.
I tried to include a change in JSF 2.2 spec related to that but there was no
success (it was considered implementation detail, really facelets algorithm has
not been standarized, because it is something complex). So, we need a custom
abstract class (AbstractFaceletCache) that extends from FaceletCache, but with
the additional methods, to allow people to create custom implementations
properly.
In my opinion, use a decoration pattern in this part does not work well,
because the logic behind the cache is not exposed by FaceletCache, or in other
words there are no explicit methods to force create facelets and store in the
cache, or refresh a facelet and so on.
Anyway, we need to create a custom AbstractFaceletCache and specify how to
implement it.
> Concurrency "feature" in FaceletCacheImpl
> -----------------------------------------
>
> Key: MYFACES-3705
> URL: https://issues.apache.org/jira/browse/MYFACES-3705
> Project: MyFaces Core
> Issue Type: Bug
> Components: General
> Affects Versions: 2.1.0
> Environment: All
> Reporter: Pasi Salminen
> Priority: Trivial
> Original Estimate: 1h
> Remaining Estimate: 1h
>
> I'm implementing my own FaceletCache which is decorating
> org.apache.myfaces.view.facelets.impl.FaceletCacheImpl by adding my own
> caching policy. When I was studying the code I'm decorating, I noticed that
> scrictly speaking it was not behaving. The problem lies in this code snippet
> (and the same for metadata facelets):
> if (_refreshPeriod != NO_CACHE_DELAY)
> {
> Map<String, DefaultFacelet> newLoc = new HashMap<String,
> DefaultFacelet>(_facelets);
> newLoc.put(key, f);
> _facelets = newLoc;
> }
> First of all, multiple concurrent modifications of _facelets map may cause
> lost updates. Think what happens when thread one copies the hashmap, updates
> local copy but before it sets the reference, thread b does the same. One
> update is now lost. In reality, the number of concurrent threads and number
> of lost updates may be much larger. The second thing is that the new
> reference set to _facelets is not quaranteed to be visible to other threads
> due to missing synchronization. To prove my concerns, I created a small test
> bench which proved my doubts and was able to show both lost updates and
> visibility problem. When I modified the map to be ConcurrentHashMap and just
> used put-method all problems vanished. So instead of
> Map<String, DefaultFacelet> newLoc = new HashMap<String,
> DefaultFacelet>(_facelets);
> newLoc.put(key, f);
> _facelets = newLoc;
> I used
> _facelets.put( key,f );
> I know it may not be a problem, possibly just causing multiple loads of same
> resource, but still I don't feel comfortable with the code behaving
> concurrency-wise incorrectly.
> BR, Paci
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
