Re: Servlet API thread safety ... was [jira] MYFACES-1558

[Dennis Byrne <de...@dbyrne.net>]

David has found a few interesting things about the FactoryFinder
implementation.  I think we can all agree that the memory leak can be fixed,
I have mixed feelings about the use of synchronization here.  The code is
invoked from many places in the app and I am mostly concerned about
performance.

If FactoryFinder.setFactory() is only invoked from the context startup
listener, or the servlet's init method, do we need synchronization?

Dennis Byrne

On 3/13/07, David Jencks (JIRA) <de...@myfaces.apache.org> wrote:
>
>
>      [
> https://issues.apache.org/jira/browse/MYFACES-1558?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
>
> David Jencks updated MYFACES-1558:
> ----------------------------------
>
>     Attachment: MYFACES-1558-jsf11.patch
>
> Here's a patch for the jsf11 branch.  I believe its the same as the patch
> for jsf 1.2 branch with generics removed, but I spent less time on it than
> the first one.  The entire jsf11 project builds fine with this.
>
> > FactoryFinder not thread safe, and has a memory leak
> > ----------------------------------------------------
> >
> >                 Key: MYFACES-1558
> >                 URL: https://issues.apache.org/jira/browse/MYFACES-1558
> >             Project: MyFaces Core
> >          Issue Type: Bug
> >          Components: General
> >    Affects Versions: 1.2.0-SNAPSHOT
> >            Reporter: David Jencks
> >         Assigned To: Dennis Byrne
> >         Attachments: MYFACES-1558-jsf11.patch, MYFACES-1558.patch
> >
> >
> > FactoryFinder has insufficient synchronization to be thread safe, and
> has a memory leak in _registeredFactoryNames.
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>


-- 
Dennis Byrne

Re: Servlet API thread safety ... was [jira] MYFACES-1558

[David Jencks <da...@yahoo.com>]

On Mar 13, 2007, at 6:46 PM, Dennis Byrne wrote:

> David has found a few interesting things about the FactoryFinder  
> implementation.  I think we can all agree that the memory leak can  
> be fixed, I have mixed feelings about the use of synchronization  
> here.  The code is invoked from many places in the app and I am  
> mostly concerned about performance.
>
> If FactoryFinder.setFactory() is only invoked from the context  
> startup listener, or the servlet's init method, do we need  
> synchronization?

I think so.  I think that hashMap.get(key) can return screwy results  
if its concurrent with hashmap.put(otherkey, value).  Using a  
ConcurrentHashMap would at least give answers that made sense, but  
the guarantees on not changing factory implementation halfway through  
would probably be weaker.

I'm sure that a totally unsynchronized implementation would pass the  
tck, so the real question is how rigorously you want to enforce the  
(probably poorly stated) requirements.  It might be worth trying to  
measure if "total synchronization" is in fact a bottleneck.

Personally I think this whole FactoryFinder thing is somewhat a bad  
idea and that obtaining the factories once when the app starts and  
injecting them into any object that could possibly want to use them  
would avoid your performance concerns and clarify the structure of  
the framework.  You may have noticed my opinion of discovery on  
another thread :-)

thanks
david jencks

>
> Dennis Byrne
>
> On 3/13/07, David Jencks (JIRA) <de...@myfaces.apache.org> wrote:
>
>      [ https://issues.apache.org/jira/browse/MYFACES-1558? 
> page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
>
> David Jencks updated MYFACES-1558:
> ----------------------------------
>
>     Attachment: MYFACES-1558-jsf11.patch
>
> Here's a patch for the jsf11 branch.  I believe its the same as the  
> patch for jsf 1.2 branch with generics removed, but I spent less  
> time on it than the first one.  The entire jsf11 project builds  
> fine with this.
>
> > FactoryFinder not thread safe, and has a memory leak
> > ----------------------------------------------------
> >
> >                 Key: MYFACES-1558
> >                 URL: https://issues.apache.org/jira/browse/ 
> MYFACES-1558
> >             Project: MyFaces Core
> >          Issue Type: Bug
> >          Components: General
> >    Affects Versions: 1.2.0-SNAPSHOT
> >            Reporter: David Jencks
> >         Assigned To: Dennis Byrne
> >         Attachments: MYFACES-1558-jsf11.patch, MYFACES-1558.patch
> >
> >
> > FactoryFinder has insufficient synchronization to be thread safe,  
> and has a memory leak in _registeredFactoryNames.
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>
>
>
> -- 
> Dennis Byrne