From: Stefan Hajnoczi <stefanha@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: armbru@redhat.com, crosa@redhat.com, qemu-devel@nongnu.org,
ehabkost@redhat.com
Subject: Re: [PATCH RFC 3/7] protocol: generic async message-based protocol loop
Date: Thu, 15 Apr 2021 10:14:15 +0100 [thread overview]
Message-ID: <YHgD52cqe/gSoa8X@stefanha-x1.localdomain> (raw)
In-Reply-To: <cf8c94de-e2fe-2416-7062-cc6991149d45@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3505 bytes --]
On Wed, Apr 14, 2021 at 01:29:40PM -0400, John Snow wrote:
> On 4/13/21 4:00 PM, Stefan Hajnoczi wrote:
> > On Tue, Apr 13, 2021 at 11:55:49AM -0400, John Snow wrote:
> One of the reasons it's split out here like this is because I also wrote a
> qtest protocol that uses the same infrastructure. I tried to keep both of
> those looking as simple as possible.
If infrastructure is needed, let's add it when it's needed but not
before then. Reviewers can't take into account qtest requirements
without seeing that code.
> > > + try:
> > > + await self._new_session(self._do_accept(address, ssl))
> > > + except Exception as err:
> > > + emsg = "Failed to accept incoming connection"
> > > + self.logger.error("%s:\n%s\n", emsg, pretty_traceback())
> > > + raise ConnectError(f"{emsg}: {err!s}") from err
> >
> > Wrapping the exception in ConnectError() obfuscates what's going on IMO.
> >
>
> It can. I will admit to you that the reason I did it was so that at a higher
> level it was possible to write things like:
>
> try:
> await qmp.connect(('127.0.0.1', 1234))
> except ConnectError:
> print("Oh no! ...")
> handle_connect_problem()
>
> while still allowing for other problems to "bubble up", for instance,
> keyboard interrupts are BaseException and won't be caught by that wrapper.
That's what "except Exception as err" is for. It allows system exiting
exceptions through.
> I adhere to this pattern fairly regularly in this draft; using "container"
> exceptions to declare a semantic problem regardless of the actual underlying
> cause, assuming that a high-level user will (probably) be unable to make
> sense of the internal details anyway.
>
> Part of the reason I do this is so that I could write in the docstrings what
> exceptions we actually expect to be raised and under what circumstances.
>
> I suppose what I don't document, though, is how to get at the "root"
> exception in these cases. You can do it:
>
> try:
> ...
> except Foo as err:
> root_err = err.__cause__
> ...
>
>
> I suppose the specific problem I wanted to solve is this: When we fail to
> connect, what exception will we see? How many types of exceptions? Which
> ones should I try to catch, and which ones should I not? Which error classes
> merit a retry, and which do not?
>
> It seemed almost impossible to enumerate, so writing informed client code
> seemed difficult or impossible.
>
> Any thoughts on this perception?
I reviewed error.py again. I was assuming the other exceptions are like
ConnectError, which would have been bad but they seem to be more
actionable. I think ConnectError is okay as a catch-all here.
requests is similar, its exceptions consist mostly of actionable
exceptions but it does have catch-all exceptions for general networking
errors:
https://docs.python-requests.org/en/latest/api/#exceptions
MultiException and the 4-level exception inheritance hierarchy in
error.py still seem complex. Is it really necessary to have
ProtocolError, RawProtocolError, and MsgProtocolError abstract classes?
Having them forces the user to make extra decisions about how to catch
exceptions. If error handling is involved, more mistakes will be made.
The requests package doesn't document the exception hierarchy. They keep
it simple with a flat list of exceptions. (RequestException is the base
class though.)
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2021-04-15 9:15 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-13 15:55 [PATCH RFC 0/7] RFC: Asynchronous QMP Draft John Snow
2021-04-13 15:55 ` [PATCH RFC 1/7] util: asyncio-related helpers John Snow
2021-04-13 15:55 ` [PATCH RFC 2/7] error: Error classes and so on John Snow
2021-04-13 15:55 ` [PATCH RFC 3/7] protocol: generic async message-based protocol loop John Snow
2021-04-13 20:00 ` Stefan Hajnoczi
2021-04-14 17:29 ` John Snow
2021-04-15 9:14 ` Stefan Hajnoczi [this message]
2021-04-13 15:55 ` [PATCH RFC 4/7] message: add QMP Message type John Snow
2021-04-13 20:07 ` Stefan Hajnoczi
2021-04-14 17:39 ` John Snow
2021-04-13 15:55 ` [PATCH RFC 5/7] models: Add well-known QMP objects John Snow
2021-04-13 15:55 ` [PATCH RFC 6/7] qmp_protocol: add QMP client implementation John Snow
2021-04-14 5:44 ` Stefan Hajnoczi
2021-04-14 17:50 ` John Snow
2021-04-15 9:23 ` Stefan Hajnoczi
2021-04-13 15:55 ` [PATCH RFC 7/7] linter config John Snow
2021-04-14 6:38 ` [PATCH RFC 0/7] RFC: Asynchronous QMP Draft Stefan Hajnoczi
2021-04-14 19:17 ` John Snow
2021-04-15 9:52 ` Stefan Hajnoczi
2021-04-20 2:26 ` John Snow
2021-04-20 2:47 ` John Snow
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=YHgD52cqe/gSoa8X@stefanha-x1.localdomain \
--to=stefanha@redhat.com \
--cc=armbru@redhat.com \
--cc=crosa@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=qemu-devel@nongnu.org \
/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.