[Aptitude-devel] Acquire considered non-threadsafe

[Daniel Burrows]

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

> >   I wonder if apt really needs an mmap'd cache here.  In fact, I wonder
> > about using sqlite -- I did a little experimentation with it and got
> > decent performance.  The big problem (from my point of view) would be
> > teaching client code to treat the database transactionally rather than
> > as a linked set of in-memory objects, but it way more cleaner and more
> > flexible than what we have now.
> Working with databases is a bit complicated. It makes development not
> really convenient.

  It's true that it's hard to keep the database contained in a library.

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

> 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

  But seriously, I don't really care whether we use exceptions or
return codes.  I guess "Google says so" is as good (or bad) a reason as
any.

  The biggest thing in favor of one or the other as far as I'm concerned
is that exceptions can abort constructors, which means you don't have to
do the dance of storing a flag to track whether an object constructed
itself properly.  On the other hand, exceptions that happen in
destructors tend to have horrible results.

> >   I would add one more item:
> > 
> >         (g) use modern C++ coding conventions (at least for new code)
> > 
> >   apt was designed in 1996 or so, and it shows.  Modern C++ is a lot
> > cleaner, easier to maintain and easier to write than the weird mishmash
> > of C and "object-oriented programming" that apt is written in.  Also,
> > a lot of modern C++ programs link against Boost, which provides a ton
> > of well-designed utility libraries for stuff that you would otherwise
> > have to write yourself, and that you'd end up writing badly.
> > (apt-pkg/contrib/ is full of that sort of thing)
> 
> We should not forget that APT has priority important, and boost libraries
> are optional. But we could switch APT to the subset of C++0x which is
> supported by GCC 4.3, the version in stable.

  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.

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



  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)

  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

  The reason I use a single background thread for all downloads is that
(a) download resources are normally global (only one network
connection), and (b) this means that clients don't have to deal with
threading themselves (other than providing a function that passes a
callback to their "main" thread) -- as far as they're concerned, the
"download complete" event arrives as a message in the main event loop.
They just have to provide a threadsafe way of passing callbacks to the
main thread, which is generally a fairly simple task.  (of course, for
clients that don't have a main loop, you can provide convenience code
to block the main thread while running a download)


  The upper level would handle what's currently mostly done by
pkgAcquire::Item, but without being "special" the way that class is (it
has friend relations with the download queue right now).  I particularly
would like to see some way of grouping together the various processes
that take place for a single "object" that's being downloaded (index,
.deb, etc); that would make it possible to show much better progress
information than is really feasible right now.

  Also, the upper level should be able to run in the background
(whether using a continuation-based coding style or in a real
background thread) without blocking execution, so that GUI programs
don't have to either poll for events or write the threading code
themselves.

  Daniel