Libguestfs - Jun 2015 - Re: [PATCH 3/5] threads: Use thread-local storage for errors.

On Sat, Jun 06, 2015 at 02:20:39PM +0100, Richard W.M. Jones
wrote:
> We permit the following constructs in libguestfs code:
> 
>   if (guestfs_some_call (g) == -1) {
>     fprintf (stderr, "failed: error is %s\n", guestfs_last_error
(g));
>   }
> 
> and:
> 
>   guestfs_push_error_handler (g, NULL, NULL);
>   guestfs_some_call (g);
>   guestfs_pop_error_handler (g);
> 
> Neither of these would be safe if we allowed the handle to be used
> from threads concurrently, since the error string or error handler
> could be changed by another thread.
> 
> Solve this in approximately the same way that libvirt does: by making
> the error, current error handler, and stack of error handlers use
> thread-local storage (TLS).
> 
> The implementation is not entirely straightforward, mainly because
> POSIX doesn't give us useful destructor behaviour, so effectively we
> end up creating our own destructor using a linked list.
> 
> Note that you have to set the error handler in each thread separately,
> which is an API change (eg: if you set the error handler in one
> thread, then pass the handle 'g' to another thread, the error
handler
> in the second thread appears to have reset itself back to the default
> error handler).  I haven't yet worked out a better way to solve this.
Do you have a feeling whether the error handler function push/pop
is a commonly used feature or not ?

In libvirt we originally had

  virGetLastError     (global error)
  virConnGetLastError (per connection error)
  virSetErrorFunc     (global error handler func)

When we introduced thread support we didn't try to make the
virConnGetLastError/SetErrorFun work. We just updated the docs
to tell applications to /never/ use them in threaded applications,
and always use the virGetLastError which was thread-local
backed. For back compatibility we do copy the error from
the global thread local into the per-connection object, so we
didn't break single-threaded apps using that old function.

IOW I think you could avoid alot of the complexity here if you
just marked the existing error handler functions as deprecated
in the context of multi-threaded usage and just introduced a
single global "get error" function that was backed by a static
allocated thread local.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

On Thu, Jun 11, 2015 at 11:27:23AM +0100, Daniel P. Berrange
wrote:
> On Sat, Jun 06, 2015 at 02:20:39PM +0100, Richard W.M. Jones wrote:
> > We permit the following constructs in libguestfs code:
> > 
> >   if (guestfs_some_call (g) == -1) {
> >     fprintf (stderr, "failed: error is %s\n",
guestfs_last_error (g));
> >   }
> > 
> > and:
> > 
> >   guestfs_push_error_handler (g, NULL, NULL);
> >   guestfs_some_call (g);
> >   guestfs_pop_error_handler (g);
> > 
> > Neither of these would be safe if we allowed the handle to be used
> > from threads concurrently, since the error string or error handler
> > could be changed by another thread.
> > 
> > Solve this in approximately the same way that libvirt does: by making
> > the error, current error handler, and stack of error handlers use
> > thread-local storage (TLS).
> > 
> > The implementation is not entirely straightforward, mainly because
> > POSIX doesn't give us useful destructor behaviour, so effectively
we
> > end up creating our own destructor using a linked list.
> > 
> > Note that you have to set the error handler in each thread separately,
> > which is an API change (eg: if you set the error handler in one
> > thread, then pass the handle 'g' to another thread, the error
handler
> > in the second thread appears to have reset itself back to the default
> > error handler).  I haven't yet worked out a better way to solve
this.
> 
> Do you have a feeling whether the error handler function push/pop
> is a commonly used feature or not ?
All the time, in our code:

$ git grep -E 'guestfs_(push|pop)_error_handler' -- *.c | wc -l
83

Of course we could change those, but it is still an API guarantee.
> In libvirt we originally had
> 
>   virGetLastError     (global error)
>   virConnGetLastError (per connection error)
>   virSetErrorFunc     (global error handler func)
> 
> When we introduced thread support we didn't try to make the
> virConnGetLastError/SetErrorFun work. We just updated the docs
> to tell applications to /never/ use them in threaded applications,
> and always use the virGetLastError which was thread-local
> backed. For back compatibility we do copy the error from
> the global thread local into the per-connection object, so we
> didn't break single-threaded apps using that old function.
>
> IOW I think you could avoid alot of the complexity here if you
> just marked the existing error handler functions as deprecated
> in the context of multi-threaded usage and just introduced a
> single global "get error" function that was backed by a static
> allocated thread local.
OK,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Thu, Jun 11, 2015 at 11:32:59AM +0100, Richard W.M. Jones
wrote:
> On Thu, Jun 11, 2015 at 11:27:23AM +0100, Daniel P. Berrange wrote:
> > On Sat, Jun 06, 2015 at 02:20:39PM +0100, Richard W.M. Jones wrote:
> > > We permit the following constructs in libguestfs code:
> > > 
> > >   if (guestfs_some_call (g) == -1) {
> > >     fprintf (stderr, "failed: error is %s\n",
guestfs_last_error (g));
> > >   }
> > > 
> > > and:
> > > 
> > >   guestfs_push_error_handler (g, NULL, NULL);
> > >   guestfs_some_call (g);
> > >   guestfs_pop_error_handler (g);
> > > 
> > > Neither of these would be safe if we allowed the handle to be
used
> > > from threads concurrently, since the error string or error
handler
> > > could be changed by another thread.
> > > 
> > > Solve this in approximately the same way that libvirt does: by
making
> > > the error, current error handler, and stack of error handlers use
> > > thread-local storage (TLS).
> > > 
> > > The implementation is not entirely straightforward, mainly
because
> > > POSIX doesn't give us useful destructor behaviour, so
effectively we
> > > end up creating our own destructor using a linked list.
> > > 
> > > Note that you have to set the error handler in each thread
separately,
> > > which is an API change (eg: if you set the error handler in one
> > > thread, then pass the handle 'g' to another thread, the
error handler
> > > in the second thread appears to have reset itself back to the
default
> > > error handler).  I haven't yet worked out a better way to
solve this.
> > 
> > Do you have a feeling whether the error handler function push/pop
> > is a commonly used feature or not ?
> 
> All the time, in our code:
> 
> $ git grep -E 'guestfs_(push|pop)_error_handler' -- *.c | wc -l
> 83
> 
> Of course we could change those, but it is still an API guarantee.
Right but you have some flexibility here. The API guarantee means
that you'll never break existing applications - which will all be
using any single connection handle from a single thread. Introducing
new rules for multi-threaded handle usage is OK, as long as you don't
regress the single-threaded handle usage.
> > In libvirt we originally had
> > 
> >   virGetLastError     (global error)
> >   virConnGetLastError (per connection error)
> >   virSetErrorFunc     (global error handler func)
> > 
> > When we introduced thread support we didn't try to make the
> > virConnGetLastError/SetErrorFun work. We just updated the docs
> > to tell applications to /never/ use them in threaded applications,
> > and always use the virGetLastError which was thread-local
> > backed. For back compatibility we do copy the error from
> > the global thread local into the per-connection object, so we
> > didn't break single-threaded apps using that old function.
> >
> > IOW I think you could avoid alot of the complexity here if you
> > just marked the existing error handler functions as deprecated
> > in the context of multi-threaded usage and just introduced a
> > single global "get error" function that was backed by a
static
> > allocated thread local.
You could maintain the push/pop behaviour too, but have it via a new
API pair that uses a static thread local for the handle stack, instead
of per handle

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|