You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
propolis_write (a future from support::send in propolis which is a wrapper around SinkExt::send()) alone
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
In
proxy_instance_serial_ws(), we construct four futures over which we willtokio::select!. In any given loop iteration, at least two of the futures are instances ofFuse::terminated()(and therefore ignored byselect!). The possible combinations of non-Fuse::terminated()futures are:nexus_read(aStreamExt::next()future) andnexus_write(aSinkExt::send()future)nexus_readandpropolis_read(a future fromInstanceSerialConsoleHelper::recv()in propolis)propolis_write(a future fromsupport::sendin propolis which is a wrapper aroundSinkExt::send()) alonepropolis_writeandnexus_writeCase 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:StreamExt::next()is cancel safe, because the equivalent method in tokio-stream's StreamExt is explicitly documented as cancel safe.SinkExt::send()is not cancel safe: ifselect!ing over aSinkExt::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 onfutures-utilto confirm / try to get it documented. EDIT: filed Documenting cancel safety ofSinkExt::send()rust-lang/futures-rs#2754)InstanceSerialConsoleHelper::recv()is cancel safe, but based on it containing multiple.awaitpoints I suspect it is not: if an item is pulled out ofself.ws_streamand 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