bridge: assorted channel-related goodies #20269
Conversation
We have a lot of places that want to accept a particular constrained set of string values. Add a helper for that.
allisonkarlitskaya
force-pushed
the
pre-bridge-send-acks
branch
2 times, most recently
from
2552dde
to
a0472e5
Compare
...in a couple of places that come to mind. This catches and fixes a minor confusing issue: we were using a default value of `msg` for the `err` option on stream channels. This is not (and has never been) a valid value, but nothing noticed that until now. Change that to be `message` which is the correct spelling as per the protocol documentation (and the unit tests).
We currently use `b''` to mean 'EOF' on `AsyncChannel.read()`, just as it means on the `read()` system call. This is a problem, because it's possible to send an empty message, and in some channels (`fsreplace1`) this even has a semantic meaning. Currently that empty message will be interpreted as EOF. Let's change things around a bit here. We now use `None` to mean `EOF`. That's going to make it more difficult to use with a while-walrus, but we can't do that anyway since we're stuck at Python 3.6. Firm up the typing a bit while we're at it.
Instead of waiting around for the channel to read() EOF when the channel is closed, let's just cancel the task that's running the `.run()` method. This simplifies channel implementation. While we're at it, let's up our exception-handling game a bit for unexpected exceptions: report those as internal errors in the close message, complete with tracebacks (but let them raise as well, so that they're also reported to stderr).
If we raise a CockpitProblem (or, for example, ChannelError) like:
raise ChannelError('internal-error') from exc
Then we get the original exception set on the ChannelError as the
`__cause__`. When we go to turn that into an error message to send, and
if the problem code was `internal-error` then give this as a traceback.
We can't do this from `__init__()` unfortunately, as the attribute gets
set after the fact. We can add a manual getter though, that handles it
at the time we return the attributes.
allisonkarlitskaya
force-pushed
the
pre-bridge-send-acks
branch
from
a0472e5
to
15c9c9b
Compare
| async def read(self): | ||
| async def read(self) -> 'bytes | None': | ||
| # Three possibilities for what we'll find: | ||
| # - None (EOF) → return b'' |
There was a problem hiding this comment.
This is a bit confusing as that's not what the code below does -- it returns None, not b''. Given that you changed everything to None, is this a leftover from a previous version?
| if isinstance(item, bytes): | ||
| if item is None: | ||
| return None | ||
| if isinstance(item, Mapping): |
There was a problem hiding this comment.
so this is a "map-ping", right?
😁
There was a problem hiding this comment.
Without look into the code, what does the ping Mapping look like?
There was a problem hiding this comment.
It just happens to be the only time we shove a dict into the queue. Here's the other end:
def do_ping(self, message: JsonObject) -> None:
self.receive_queue.put_nowait(message)so if we find a dict in there, we can be pretty sure that it's a ping and we should pong it...
| @@ -114,7 +115,7 @@ class FsReadChannel(GeneratorChannel): | |||
|
|
|||
| def do_yield_data(self, options: JsonObject) -> GeneratorChannel.DataGenerator: | |||
| path = get_str(options, 'path') | |||
| binary = get_str(options, 'binary', None) | |||
| binary = get_enum(options, 'binary', ['raw'], None) is not None | |||
There was a problem hiding this comment.
So the binary option can be bool or 'raw' that is rather confusing. Maybe an idea we deprecate the raw option?
There was a problem hiding this comment.
It can never be bool. This is a common source of confusion. It must always be either absent or the literal string "raw"
There was a problem hiding this comment.
Then this is an issue maybe, since:
111 │ const options = {
112 │ method: "GET",
113 │ path: client.VERSION + "libpod/containers/" + this.props.containerId + "/logs",
114 │ body: "",
115 │ binary: true,
116 │ params: {
117 │ follow: true,
118 │ stdout: true,
119 │ stderr: true,
120 │ },
121 │ };
There was a problem hiding this comment.
Oh it's the filesystem channel sigh. Well confusing regardless :)
| if isinstance(item, bytes): | ||
| if item is None: | ||
| return None | ||
| if isinstance(item, Mapping): |
There was a problem hiding this comment.
Without look into the code, what does the ping Mapping look like?
#20160 builds on this. I'll push a tiny fixup there. |
Here's some assorted cleanups and improvements that will help with #20160. Let's merge these first, since they're not directly related (and are big enough that they might cause regressions in their own right).