Feature: A new `RemoteChildOwned` struct to avoid self-ref issue

[NobodyXu]

From https://github.com/openssh-rust/openssh-sftp-client/pull/65#issuecomment-1533280291 :

If it possible for Sftp to hold a session and spawn command (which means hold on RemoteChild) internally? So that users don't need to hold session, remote_child and Sftp at the same time. To avoid RemoteChild been dropped earlier than Sftp, users like opendal have to use owning_ref tricks.

openssh_sftp_client::Sftp now has a new method new_with_auxiliary to hold any data and @Xuanwo & @silver-ymz is currently using a workaround I proposed by using a Boxed future.

But I think it's still better to have a RemoteChildOwned to avoid the self-ref issue, it's pretty bad for anybody using it with openssh_sftp_client.

Alternatively, we can wait for the new proxy-mode to openssh-mux-client https://github.com/openssh-rust/openssh-mux-client/issues/6 , which will enable you to use stdin/stdout/stderr while dropping Session and RemoteChild.

It will also support windows.

Unfortunately, it was blocking on tokio_util::sync::WaitForCancellationFutureOwned, so I switched to other projects and I kind of forgot it. I will restart working on it, but it will takes quite some time before it's ready to use.

[jonhoo]

I think this makes a lot of sense! The way to go about it is probably to start carrying around Arc<Session> instead of just Session, and then have a variant of RemoteChild that clones that Arc<Session> instead of keeping a &'s Session inside. Should be a pretty trivial change I think beyond having to duplicate a bunch of the RemoteChild code. If you can think of a clever way in which we can avoid duplication much of the code, that'd be nice, but not a requirement.

Do you think we should also deprecate RemoteChild with the plan to make RemoteChildOwned the only RemoteChild in the future? My main concern would be that people end up unexpectedly leaving sessions open because they're still storing a RemoteChild somewhere, when in reality they might want the borrow checker to tell them that that is the case.

[NobodyXu]

I think this makes a lot of sense! The way to go about it is probably to start carrying around Arc<Session> instead of just Session, and then have a variant of RemoteChild that clones that Arc<Session> instead of keeping a &'s Session inside. Should be a pretty trivial change I think beyond having to duplicate a bunch of the RemoteChild code. If you can think of a clever way in which we can avoid duplication much of the code, that'd be nice, but not a requirement.

That's definitely doable!

Our underlying process-impl and native-mux-impl does not hold any reference to Session and most of the method (except for RemoteChild::{new, session} does not use the session reference at all.

Do you think we should also deprecate RemoteChild with the plan to make RemoteChildOwned the only RemoteChild in the future? My main concern would be that people end up unexpectedly leaving sessions open because they're still storing a RemoteChild somewhere, when in reality they might want the borrow checker to tell them that that is the case.

I think keeping both is doable, there is no need to deprecate them.

[NobodyXu]

Though it will indeed add some overhead, since we now need to store SessionImp in an Arc.

[masklinn]

Should be a pretty trivial change I think beyond having to duplicate a bunch of the RemoteChild code. If you can think of a clever way in which we can avoid duplication much of the code, that'd be nice, but not a requirement.

I assume I'm missing something obvious but couldn't RemoteChild store a T: AsRef<Session> (or T: Deref<Target=Session> I'm never quite sure which is the correct one)? And that would really only even be for the session method.

Although it would change the signature of RemoteChild itself so maybe the concrete struct should be something else and RemoteChild<'a> would become be an alias for ???Child<&'a Session>.

[NobodyXu]

Thanks @masklinn !

I think that's feasible and would require less code. I will have a try of this and I also welcome any PR for this!