openssh-rust / openssh

Scriptable SSH through OpenSSH in Rust
Apache License 2.0
232 stars 35 forks source link

Confusing mix of references and absolutes in various function calls #163

Open CowBoy4mH3LL opened 3 weeks ago

CowBoy4mH3LL commented 3 weeks ago

I see a few types (not comprehensive though) of functions defined in the library depending on their input and output -- those with input: absolute, references, Arc -- those with output: absolute, references, Arc

Given that, I am facing issues when using reference/Arcs outputted by some functions as arguments of others that take absolutes as inputs. Here is an example

struct Some{
_session: Arc<Session>,
...
}

fn some_new() -> Some{
  ...
  let session:Arc<session> = 
          Arc::new(
              Session::connect(user_at_server, KnownHosts::Accept)
              .await.expect("Could not connect via SSH!!")
          );
  ..
  Self{
  _session: session,
...
  }

fn some_other(){
   ...
   session.clone().close();
...
}
}
....

From the above, the compiler does not likesession.clone().close() because clone() returns another Arc and Session does not implement Copy/Clone and close() takes an absolute self as argument.

Is this an already discussed case, or am I missing something really obvious :) ?

NobodyXu commented 3 weeks ago

Session::close would need to take ownership, to ensure that nothing can use it afterwards.

I think you could use Arc::into_inner to convert Arc<Session> to an owned Session, then you could use Session::close

BTW, Session implements Drop, so even if you don't call Session::close it'd close it on drop, the difference is that Session::close would return an error on failure.

CowBoy4mH3LL commented 3 weeks ago

Firstly, thanks for the super-fast response.

I had tried into_inner, try_unwrap, etc. in my own drop i.e. Drop the Some struct which I suspect would be dropped before the inner _session is dropped -- somehow none of these work, i.e. they end up with a none/error output when unwrapping the inner session. Would really appreciate it if you can give it a shot at your end/playground. :)

line 93: Arc::<Session>::into_inner(self._session.clone()).unwrap();
thread 'cmd::test::test' panicked at src/cmd.rs:93:59:
called `Option::unwrap()` on a `None` value

Secondly, I am connecting to an openssh server on a windows box and regular drop does not shut the session. So I suspected some form of error, and hence went on this route.

NobodyXu commented 3 weeks ago

So I suspected some form of error, and hence went on this route

About that, I think it's time for me to add optional tracing support to log these errors in drop.

NobodyXu commented 3 weeks ago

Drop the Some struct which I suspect would be dropped before the inner _session is dropped -- somehow none of these work, i.e. they end up with a none/error output when unwrapping the inner session.

Maybe there're have some Command/Child/ChildStd* that reference the Arc<Session>?

CowBoy4mH3LL commented 3 weeks ago

Drop the Some struct which I suspect would be dropped before the inner _session is dropped -- somehow none of these work, i.e. they end up with a none/error output when unwrapping the inner session.

Maybe there're have some Command/Child/ChildStd* that reference the Arc<Session>?

Indeed -- that's just a command that builds of a session -- typical case

let mut cmd = session
        .clone()
        .arc_raw_command(
        program
        .display().to_string()
        );

But I tired by hashing it out -- issue remains Anyhow, the way I see it -- Rust puts this incredible burden of being able to remember data flows statically which becomes painful at times -- as such this issue probably occurs due to the interplay between ownership and referencing.

Are the functions such as close, disconnnect, etc. purposefully built to take ownership of self? If not, can we take references instead?

CowBoy4mH3LL commented 3 weeks ago

Ok so -- just noticed the sessions are going into TIME/CLOSE/FIN WAIT -- so maybe the automatic drop->close is working -- but again I will make a double check later.

CowBoy4mH3LL commented 3 weeks ago

BTW -- does the child automatically disconnect when it goes out of scope? i.e. say I execute commands in a loop and each call execution is performed in a block of its own -- will each child be disconnected after the block ends?

NobodyXu commented 3 weeks ago

Are the functions such as close, disconnnect, etc. purposefully built to take ownership of self? If not, can we take references instead?

@CowBoy4mH3LL yes, that's definitely possible!

TBH, openssh initially supports only reference, and then we added support of Arc/Rc for certain workflow.

NobodyXu commented 3 weeks ago

BTW -- does the child automatically disconnect when it goes out of scope? i.e. say I execute commands in a loop and each call execution is performed in a block of its own -- will each child be disconnected after the block ends?

Yes, the remote will kill it.