[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [equinox-dev] Thread Safety Issues
|
On Thu, Sep 18, 2008 at 07:45:05AM -0500, Thomas Watson wrote:
>
> Rob,
>
> What codebase are you working off of? I want to make sure you are using
> the latest from HEAD (Galileo/3.5 release). Major thread safety rework is
> going to have to happen in 3.5. If we have isolated fixes we can consider
> porting them to the 3.4.x release. I want to focus on the 3.5 code base
> first.
>
> Tom
Tom,
I'm working against CVS HEAD.
Rob
>
>
>
>
>
> From: Rob Harrop <rob.harrop@xxxxxxxxxxxxxxxx>
>
> To: Equinox development mailing list <equinox-dev@xxxxxxxxxxx>
>
> Date: 09/18/2008 07:20 AM
>
> Subject: Re: [equinox-dev] Thread Safety Issues
>
>
>
>
>
>
> 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
> _______________________________________________
> 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
