Re: [equinox-dev] Thread Safety Issues

[Rob Harrop <rob.harrop@xxxxxxxxxxxxxxxx>]

On Thu, Sep 18, 2008 at 06:30:40AM -0500, Thomas Watson wrote:
> 
> Hi Rob,
> 
> Thanks for your continued help in this area.  I have not had time to look
> into the details of your patches but your approach sounds reasonable.  To
> answer your question about the resolver.  The original design was intended
> to allow a resolver implementation to be plugged in.  This has never been
> realized.  At this time I am happy to keep resolver in the same code base
> as the State implementation to solve the threading issues.
> 
> Tom

I'm glad to help. No rush on checking out the patches - I'm running our test suite
against a build I made locally so I'm can still work and test. I'll continue to
work on StateImpl and update the patches as soon as I have a working solution.

> 
> 
> 
> 
>                                                                        
>   From:       Rob Harrop <rob.harrop@xxxxxxxxxxxxxxxx>                 
>                                                                        
>   To:         Equinox development mailing list <equinox-dev@xxxxxxxxxxx>
>                                                                        
>   Date:       09/18/2008 05:50 AM                                      
>                                                                        
>   Subject:    Re: [equinox-dev] Thread Safety Issues                   
>                                                                        
> 
> 
> 
> 
> 
> Tom,
> 
> I updated the patch for the BundleRepository issue. After looking at it in
> detail
> I decided the best way to go was to simply synchronize on the public
> methods of BR
> and allow clients to participate in the client locking protocol. With this
> approach
> this is no need to stop using Framework.bundles as a lock for other
> composite operations.
> 
> I started working on StateImpl and general resolver thread-safety and I
> have few questions.
> Firstly, I notice that StateDelta.getState() and StateImpl.compare() are
> unused other than
> in tests. Are they intended for use by other codebases?
> 
> Secondly, the biggest challenge with making StateImpl threadsafe is around
> its interaction
> with its Resolver. I really wanted to avoid making calls to the Resolver
> while holding any
> locks but this seems like it will be impractical, and in fact won't result
> in StateImpl being
> fully safe. I notice already that a few calls to the Resolver are made
> whilst holding the
> StateImpl lock (linkDynamicImport for example). Is this something you are
> happy to keep
> since the Resolver is basically in the same codebase?
> 
> Rob
> 
> On Wed, Sep 17, 2008 at 07:02:08AM -0500, Thomas Watson wrote:
> > > [equinox-dev] Thread Safety Issues
> > >
> > > Rob Harrop
> > >
> > > to:
> > >
> > > equinox-dev
> > >
> > > 09/16/2008 11:38 AM
> > >
> > >
> > > Chaps,
> > >
> > > I've created a few bug reports (with patches) related to thread
> > > safety issues we have found while using Equinox in SpringSource dm
> > > Server. I felt that it wasn't appropriate to create massive patches
> > > so I focused instead on point fixes, but I wanted to briefly outline
> > > what I think is a fuller solution to the highlighted issues.
> >
> > Thanks Rob.  I agree that work is needed here.  I opened another plan
> > bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=247636 to track the
> > various bugs you and Glyn Normington have opened.  We plan to work
> > on bundle lifecycle thread safety issues in the 3.5 release cycle.
> >
> > >
> > > So far we have identified three main issues:
> > >
> > >  * BaseStorage.getNextBundleId() performs unsafe increment
> > >     (https://bugs.eclipse.org/bugs/show_bug.cgi?id=247520)
> > >  * StateImpl access bundleDescriptions in an unsafe manner
> > >    (https://bugs.eclipse.org/bugs/show_bug.cgi?id=247522)
> > >  * Framework access BundleRepository in an unsafe manner
> > >    (https://bugs.eclipse.org/bugs/show_bug.cgi?id=247521)
> > >
> > > For the first issue, I think the patch I supplied is fine, the
> > > solution there is pretty straightforward.
> > >
> > > For the StateImpl issue, I attacked only the issue around accesses
> > > to the bundleDescriptions field, but I think that accesses to all
> > > fields need to be protected. Moreover, I think that all the state of
> > > StateImpl needs to be protected by the same lock to guarantee atomic
> > > updates of the internal data structures.
> >
> > I agree with the analysis.  I looked into this a few weeks ago and
> > identified that the State object needs to be protected when accessed
> > by the framework.  The tricky part here is the pervasive access to
> > the State object in the Framework and the risk of deadlock.
> >
> > >
> > > In Framework, all calls to BundleRepository.add were not guarded by
> > > the bundles lock. I made sure that all accesses to bundles are
> > > correctly (I think) guarded. However, I feel a better approach is to
> > > make BundleRepository itself threadsafe. The JavaDoc of BR currently
> > > states that it must be synchronized before access but obviously it
> > > is easy to forget, as is the case currently. Also, I notice that
> > > Framework.bundles is used as a monitor for a lot of operations that
> > > don't directly manipulate its state and it can also escape control
> > > of Equinox via Framework.getBundles(). This is deadlock prone.
> > > Ideally, the BundleRepository would remain an Equinox internal data
> > > structure, maintain its own thread safetry and not be used as a
> > > monitor for other operations. Replacing the use of Framework.bundles
> > > for synchronization with something like Framework.bundlesMonitor
> > > will help here.
> >
> > I agree.  Something like this is needed.
> >
> > >
> > > I haven't had to chance to explore the code further, but I'll raise
> > > any other issues as I find them.
> > >
> > > If you want to make these larger thread-safety changes, I am happy
> > > to submit patches that address them.
> >
> > Thanks Rob!  I will be glad to work with you on these issues.  It will
> > really help to have SpringSource help.  Typical Eclipse scenarios
> > manage bundle lifecycles (install,uninstall,update,refresh) from a
> > single thread so we do not exercise thread safety in this area.
> >
> > Thanks for your help.
> >
> > Tom.
> >
> > >
> > > Regards,
> > >
> > > Rob
> > >
> > > --
> > > Rob Harrop
> > > SpringSource
> > > _______________________________________________
> > > equinox-dev mailing list
> > > equinox-dev@xxxxxxxxxxx
> > > https://dev.eclipse.org/mailman/listinfo/equinox-dev
> > _______________________________________________
> > equinox-dev mailing list
> > equinox-dev@xxxxxxxxxxx
> > https://dev.eclipse.org/mailman/listinfo/equinox-dev
> 
> 
> --
>  -- Rob Harrop
> _______________________________________________
> equinox-dev mailing list
> equinox-dev@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/equinox-dev
> 




> _______________________________________________
> equinox-dev mailing list
> equinox-dev@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/equinox-dev


-- 
 -- Rob Harrop