Re: [equinox-dev] Thread Safety Issues

[Rob Harrop <rob.harrop@xxxxxxxxxxxxxxxx>]

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