[Aptitude-devel] Acquire considered non-threadsafe

Mon Nov 23 23:00:21 UTC 2009

On Mon, Nov 23, 2009 at 07:41:42AM -0800, Daniel Burrows wrote:
> On Thu, Nov 19, 2009 at 06:09:42PM +0100, Julian Andres Klode <jak at debian.org> was heard to say:
> > On Thu, Nov 19, 2009 at 04:31:13AM -0800, Daniel Burrows wrote:
> > >   Also, apt2?  When did that happen?
> > APT2 will be a package management library + applications written in Vala,
> > but at the moment it's just a few parsers and a simple acquire system. And
> > it's intented to be cross-distro, just like PackageKit. I just had no better
> > name when I started it. It's currently MIT-licensed, but may switch to the
> > LGPL-2.1+ soon (as this is what all its dependencies like glib use).
> 
>   Hm.  I'm pretty skeptical about writing a system tool like this in a
> high-level language.  Both from the point of view of dependency
> fragility and because you want it to be easy to access the functionality
> from other languages.  In fact, I've occasionally wondered if it makes
> sense for the public API of an apt successor to be pure C (even if the
> implementation is C++ behind the scenes).  C++ ABIs are obnoxiously
> fragile and poorly defined, for one thing. (although very convenient if
> you're writing in C++)

Vala compiles to C, and provides a C-API comparable to other GObject libraries
such as GTK+. You get a C library with a header file and everything, and you
can write applications using the library in C. For now, dependencies are
	libglib2.0-0
	libgee1 (container classes)
	libsoup2.4-1 (HTTP/HTTPS acquire part)

The latter one could be replaced with a limited http downloader for embedded
systems; which only depends on libglib2.0-0.

> 
> > > 
> > > > 	(f) storing errno inside the error handling objects,
> > > >             for bindings (nice to have errno).
> > > 
> > >   Please, please, kill the global error queue.  This is a disaster from
> > > a threadsafety point of view, but also just from a general design
> > > perspective.  There's way too much code that does
> > > 
> > >     if(!_error->PendingError())
> > >       fail();
> > > 
> > >   when the error could have been caused by something entirely unrelated
> > > to what this code is interested in.
> > > 
> > >   Put error-handling oun either a return-code basis or an exception
> > > basis, but not this wird intermediate thing where random errors cause
> > > failure.  If you want to know about inner errors, use a logging
> > > framework (I prefer log4cxx).
> > As far as I can tell from the documentation, the APT error handling should
> > only be used for boolean functions. If the function has an error, it pushes
> > a message to the error stack and returns false. If we used it this way only,
> > we would have no multi-threading problems.
> 
>   Many uses of PendingError() in apt-pkg need to be fixed then.  The
> problem is, when it's used this way, you can't tell which errors the
> code is actually supposed to detect, which makes it tricky to pull the
> call out safely (you'll end up changing behavior).  Would you be OK with
> a patch that eliminated these usages anyway?  What if I also changed the
> code to use return codes in places where it's obvious which error(s)
> is/are being checked?
> 
>   Grep shows me 38 uses of PendingError() in apt-pkg.  At a quick
> glance, it looks like most of those are problematic.  I didn't look
> closely enough to determine how hard each one will be to fix.
> 
>   I would like to emphasize that this is not just a multi-threading
> problem; any time that the error queue might be in an unknown state (for
> instance, after pkgAcquire::Pulse() is invoked), you could see spurious
> failures.
OK.

> 
> > Exceptions are a bit tricky in C++ as they can happen unexpectedly. Google's
> > C++ style guide says no; so we should probably also say no.
> 
>   Do we work for Google?  I wanna get paid in that case :P
> 
Just a reference. 

>   OTOH, most of the Boost libraries are compile-time-only, so aside from
> bootstrapping this isn't necessarily a problem (assuming we don't use
> iostreams or anything like that); policy explicitly excludes build-time
> dependencies from priority rules.  On the first hand, a good shared_ptr
> implementation, which is in C++0x IIRC, would be a nice starting point;
> most of the other stuff I use from Boost is more special-purpose.  And
> C++0x has some other really nice (and badly needed) stuff in it.
shared_ptr is also available in std::tr1::shared_ptr, at least in current
GCC versions. We could even require a GCC with the C++0x stuff we need
an build with --std=c++0x.

> 
> > Some kind of a roadmap:
> > 
> >     * identify stuff we don't use anymore and remove it, eg.
> >       apt-pkg/vendor*, apt-inst/database*, apt-inst/deb/dpkgdb*.
> >     * drop compatibility for GCC < 4.3, and other stuff older
> >       then what is included in Debian Lenny.
> >     * rework the buildsystem using CMake
> 
>  (ABI breaking stuff)
>       * Write a more organized download system. (see below)
> 
>   Maybe also figure out a style guide for C++ going forwards (e.g.,
> recommend not using exceptions, using smart pointers, using STL
> containers, etc).
Or copy Google's style guide and remove the stuff we don't like; and
adjust it to our coding standards. Their style guide is very complete,
and thus a good base.

>   As far as the download system goes, I'd prefer to see an explicit
> two-level design, where the lower level downloads URIs without caring
> what they "mean", and the upper level manages "jobs" that download
> multiple URIs (such as "update some packages"), using the lower level
> object to do the actual download.  This would make it a lot easier for
> code linked against apt to make use of apt's nice download functionality
> without having to jump through hoops just to download a URI.  (one of my
> pet peeves)
Yes.

> 
>   In aptitude, I have built an internal API that provides a low-level
> URI downloader running in a background thread.  Currently it's based on
> Acquire, but with a little work it could replace Acquire instead
> (at first glance, the Acquire main loop and the download queue management
> would need to be incorporated).  I think it's a good example of the
> simple interface I'm thinking about, anyway:
> 
> http://hg.debian.org/hg/aptitude/head/file/b8a7fc61b13c/src/generic/apt/download_queue.h

I'm missing a few parts like a target filename, but maybe this is defined
somewhere else?

-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.