[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [equinox-dev] Thread Safety Issues

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