[propolis] address cancel-safety issues with InstanceSerialConsoleHelper::recv #435
Conversation
Created using spr 1.3.4
|
So sadly this means that while a migration is happening, the other select branches (and therefore progress) will be completely blocked. There is likely a way to write |
I think it's actually okay to block the other select branch in this case - if a Text frame has been sent, it means the rest of the migration has completed already, and we shouldn't send characters any more until the InstanceSerialConsoleHelper's had its ws_stream replaced with the connection to the destination instance. |
There was a problem hiding this comment.
WIP looks good so far. I'm currently imagining that tests might look like one demonstrating that putting a future that effectively does x = ws.recv().process() => { ... } in the select! causes the message to be lost outright, and one where x = ws.recv() => { x.process() ... } doesn't, where to avoid flaking we can force the cancellations to be predictable by having the 'destination server' be some mock that'll accept the client.instance_serial() call's connection and stall until we explicitly tell it to finish the handshake.
lib/propolis-client/src/lib.rs
Outdated
| /// # Cancel safety | ||
| /// | ||
| /// This method is cancel-safe and can be safely used in a `select!` | ||
| /// loop. However, to , and that portion is not cancel-safe. |
There was a problem hiding this comment.
I suppose this should say something like However, [InstanceSerialConsoleMessage::process] must be awaited to retrieve the inner [WSMessage], and that portion is not cancel-safe.?
Created using spr 1.3.4
sunshowers
changed the title
sunshowers
deleted the
sunshowers/spr/wip-propolis-address-cancel-safety-issues-with-instanceserialconsolehelperrecv
branch
We regressed on this in #435. Ensure that it's Send, and statically assert it.
In #435 we moved all connection management to inside the `InstanceSerialConsoleHelper`. For testing purposes in omicron we need to expose something that can build a console helper out of any stream. (We were already using this code for tests in propolis, but omicron has its own tests.)
TODO:
See #434 and inline comments.