Encode semantics of Session::keepalive_send in return type

[emesterhazy]

Issue

The current implementation of Session::keepalive_send returns Result<u32, Error>. Users might suspect that if they receive an Ok(seconds) they should act on it and send another keep alive message after seconds has elapsed.

However, this is not the case if keepalive is disabled for the session since Session::keepalive_send will return Ok(0) indicating that keepalive is not set. Here's where that happens in libssh2:

https://github.com/libssh2/libssh2/blob/a88a727c2a1840f979b34f12bcce3d55dcd7ea6e/src/keepalive.c#L62-L66

    if(!session->keepalive_interval) {
        if(seconds_to_next)
            *seconds_to_next = 0;
        return 0;
    }

Possible Solutions

The most lightweight thing we can do is update the documentation for Session::keepalive_send so that it clearly indicates that users will get an Ok(0) back if keep alive is not set.

Alternatively, if we're alright with a breaking change for the next minor version release we could change the return type to Result<Option<NonZeroU32>, Error>, or Result<Option<Duration>, Error> and set the option to None if the C api returns zero. I think this is the best way to prevent accidental misuse of the API, but it does require a breaking change.

[yodaldevoid]

This is an excellent idea. I will make a point to include the breaking change in the next minor release.

Until then, I will update the documentation.

[Jeidnx]

If this is still on the list to be implemented, i would suggest encoding the exact responses in the type system, instead of just using an Option. Something like this:

enum KeepaliveResponse {
  Seconds(u32), // Alternatively this could also be a `Duration`.
                // We should make sure this is the same type as
                // the `interval` passed to `set_keepalive` imo.
  NoKeepAliveSet,
}

fn keepalive_send(&self) -> Result<KeepaliveResponse, Error>