All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, Kevin Wolf <kwolf@redhat.com>,
	qemu-block@nongnu.org, hreitz@redhat.com, aesteve@redhat.com,
	nsoffer@redhat.com, vsementsov@yandex-team.ru
Subject: Re: Reference-counting and finalizers that can fail are uneasy partners (was: [PATCH 0/4] qemu-img: Fix exit code for errors closing the image)
Date: Wed, 22 Feb 2023 12:54:46 +0000	[thread overview]
Message-ID: <Y/YQlq3NXVMkCyzP@redhat.com> (raw)
In-Reply-To: <87ttzdoqca.fsf_-_@pond.sub.org>

On Wed, Feb 22, 2023 at 01:08:05PM +0100, Markus Armbruster wrote:
> A half-baked thought has been sloshing around in my head.  Perhaps I can
> bake it some more by writing it up.
> 
> Reference-counting and finalizers that can fail are uneasy partners.
> 
> When managing lifetimes manually, you control where finalization
> happens.  When finalization can fail, you're as empowered as you could
> be to make it fail in a place where you can handle the failure sensibly.
> 
> Manual resource management is tedious and error prone, and that's a
> serious problem.  Garbage collection takes it off your hands.  Good.
> But now finalization happens at some future time, and in garbage
> collection context.  Fine when finalization's side effects are
> sufficiently harmless.  But what if finalization can fail?  We trade one
> serious problem (manual resource management) for another one (handling
> finalization failures).
> 
> Reference counting is slightly different.  Here, finalization can only
> happen at unref, which means you retain more control than with garbage
> collection.  However, we do need unrefs in places where we can't
> sensibly handle failure.  For instance, when code operates on an object
> whose reference count can be dropped concurrently, we need to guard with
> a ref/unref bracket to keep the object alive while the code is messing
> with it.
> 
> The only way out I can see is to systematically avoid finalizers that
> can fail, by extracting the part that can fail into a shutdown method,
> to be called in a suitable context, and before finalization.

Yes, I concur with pretty much everything you say above.

Since finalizers can occur in any context the logic that runs in
them needs to be quite clearly defined and free of side effects
or unpredictable failure scenarios.

I would probably go further and say that finalizers need to be
able to execute in finite time, so that callers do not have
execution of their thread blocked arbitrarily if they happen to
be one the one that releases the last reference.

Finalizers should be releasing resources that are already in
a "safe" state. Releasing memory, decrementing references,
unregistering callbacks, are typical safe things.

Performing I/O is a clearly a bad idea / inappropriate for
a finalizer by this standard.

Garbage collection vs reference counts is tangential to this
problem, as you say, they'll both share the same problem we're
facing.

> Yes, this takes us back to manual resource management, only we manage
> shutdown instead of death.
> 
> Finalizing something that has not been shut down would be a programming
> error.  A recoverable one, I guess; we can have finalize attempt to shut
> down then, and if it fails, just weep into the logs and move on.

This is approximately what I did with QIOChannel.  There is a
qio_channel_close() method that is best practice to invoke to
release resources assoociated with the channel, possibly flushing
pending I/O, and reporting failures via an Error **errp.

If this is not called, however, the finalizer will call close
on your behalf, discarding errors. It'll probably be OK much of
the time, and if we find it isn't, then the missing explicit
close call needs to be addressed.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



  reply	other threads:[~2023-02-22 12:55 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-12 19:14 [PATCH 0/4] qemu-img: Fix exit code for errors closing the image Kevin Wolf
2023-01-12 19:14 ` [PATCH 1/4] qcow2: Fix theoretical corruption in store_bitmap() error path Kevin Wolf
2023-01-13  7:30   ` Philippe Mathieu-Daudé
2023-01-13 10:45     ` Kevin Wolf
2023-01-13 17:37       ` Philippe Mathieu-Daudé
2023-01-12 19:14 ` [PATCH 2/4] qemu-img commit: Report errors while closing the image Kevin Wolf
2023-01-12 19:14 ` [PATCH 3/4] qemu-img bitmap: " Kevin Wolf
2023-01-13  7:32   ` Philippe Mathieu-Daudé
2023-01-12 19:14 ` [PATCH 4/4] qemu-iotests: Test qemu-img bitmap/commit exit code on error Kevin Wolf
2023-01-13  7:30 ` [PATCH 0/4] qemu-img: Fix exit code for errors closing the image Markus Armbruster
2023-01-13 11:29   ` Kevin Wolf
2023-02-14 20:09     ` Vladimir Sementsov-Ogievskiy
2023-02-15 13:07     ` Markus Armbruster
2023-02-15 20:50       ` Vladimir Sementsov-Ogievskiy
2023-02-22 12:08       ` Reference-counting and finalizers that can fail are uneasy partners (was: [PATCH 0/4] qemu-img: Fix exit code for errors closing the image) Markus Armbruster
2023-02-22 12:54         ` Daniel P. Berrangé [this message]
2023-01-17 11:24 ` [PATCH 0/4] qemu-img: Fix exit code for errors closing the image Hanna Czenczek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y/YQlq3NXVMkCyzP@redhat.com \
    --to=berrange@redhat.com \
    --cc=aesteve@redhat.com \
    --cc=armbru@redhat.com \
    --cc=hreitz@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=nsoffer@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vsementsov@yandex-team.ru \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.