channel_success and request_success are not ergonomic

[jeromegn]

(Sorry, I submitted this too fast with no comment)

We just got bitten by not using channel_success on a channel request for a client that expected a reply (want_reply: true).

I'm wondering why this has to be manually called. Could it be instead inferred from the Result of each handler function?

[Eugeny]

There's some discussion about it in #348

[lgmugnier]

I wanted to add this comment to the issue instead of the PR since it explains our specific situation and offers a reliable workaround.

We ran into this. We setup a very simple client/server and when the client executed these lines:

let mut channel = handle.channel_open_session().await?;
channel.request_shell(false).await?;
channel.data(command.as_bytes()).await?;
channel.eof().await?;

The Server responds to shell requests thusly:

async fn shell_request(
    &mut self,
    channel: ChannelId,
    session: &mut russh::server::Session,
) -> Result<(), Self::Error> {
    session.channel_success(channel)?;
    Ok(())
}

The proxy we have in between the two would throw an error saying that it had never received a CHANNEL_SUCCESS (99). We discovered that the first message we got after channel.request_shell(true).await?; was consistently a CHANNEL_OPEN_CONFIRMATION (91) which satisfied the request for response in the request_shell(true) call but not in the protocol. A CHANNEL_SUCCESS message was always the one that followed.

Side note: I wonder if the CHANNEL_OPEN_CONFIRMATION message should be the responsibility of the handle.channel_open_session().await? call?

This is the workaround we came to. Even if it requires more understanding of individual SSH messages (and it's more code), it's a reliable solution. You can also specifically handle other common messages like ChannelMsg::Data or ChannelMsg::ExitStatus.

let mut channel = handle.channel_open_session().await?;
channel.request_shell(); // sends a REQUEST_SHELL message

loop {
    match channel.wait().await {
        Some(ChannelMsg::Success) => break, // got the shell, proceed
        Some(ChannelMsg::Failure) => panic!("CHANNEL FAILED TO OPEN"),
        Some(_) => {} // ignore everything else
        None => panic!("CHANNEL WAS CLOSED"),
    }
}

// now you can send the command
channel.data(command.as_bytes()).await?;
channel.eof().await?;