Re: [equinox-dev] Thread Safety Issues

[Rob Harrop <rob.harrop@xxxxxxxxxxxxxxxx>]

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.
>

That's great. Any help I can provide in the timeframe I will.
 
> >
> > 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.
>

This is quite an interesting one. I haven't looked into this in detail
yet, but I'll spend some time running through the call chains to
figure out how StateImpl can be made safe as a whole and I'll run
from there. Is it a use case to allow third-parties to plug-in custom
Resolver implementations?

 
> >
> > 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.

No worries - I'm glad to help. I've budgeted some time to work on these
problems so I should be able to submit fuller patches over the coming
weeks.

How do patches migrate into nightly builds? For example, when should we 
expect to see the getNextBundleId fix in a nightly so we can run our 
test suite against it.

> 
> 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