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

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



Inactive hide details for Rob Harrop ---09/18/2008 05:50:27 AM---Tom,Rob Harrop ---09/18/2008 05:50:27 AM---Tom,


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


GIF image

GIF image