Skip to content

proxy_instance_serial_ws() uses select! with non-cancel-safe future(s) #3356

@jgallagher

Description

@jgallagher

In proxy_instance_serial_ws(), we construct four futures over which we will tokio::select!. In any given loop iteration, at least two of the futures are instances of Fuse::terminated() (and therefore ignored by select!). The possible combinations of non-Fuse::terminated() futures are:

  1. nexus_read (a StreamExt::next() future) and nexus_write(a SinkExt::send() future)
  2. nexus_read and propolis_read (a future from InstanceSerialConsoleHelper::recv() in propolis)
  3. propolis_write (a future from support::send in propolis which is a wrapper around SinkExt::send()) alone
  4. propolis_write and nexus_write

Case 3 is fine because there is only one possible future for select! to choose, so there is no possibility for cancellation. But in the other three cases, select! will cancel one of the two futures once it chooses one. In terms of cancel safety, none of these futures is explicitly documented; however:

  • I believe (hope?) that StreamExt::next() is cancel safe, because the equivalent method in tokio-stream's StreamExt is explicitly documented as cancel safe.
  • I believe SinkExt::send() is not cancel safe: if select!ing over a SinkExt::send() future and a different arm is chosen, the item being sent to the sink will be lost. (A reproducer; I'll also file an issue on futures-util to confirm / try to get it documented. EDIT: filed Documenting cancel safety of SinkExt::send() rust-lang/futures-rs#2754)
  • I'm not sure whether InstanceSerialConsoleHelper::recv() is cancel safe, but based on it containing multiple .await points I suspect it is not: if an item is pulled out of self.ws_stream and the future is dropped at a later await point, I believe the item will be lost.

I'm not sure at the moment how to address this; I'm just skimming through omicron's select!s looking for suspicious cancel safety issues. CC @lifning

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions