Session in 0.4.0 no longer Send

[usbpc]

In version 0.3.3 the Session struct implemented the Send trait unfortunately since in 0.4.0 it's using a std::rc::Rc to store the SessionInner, std::rc::Rc is very explicitly not Send, therefore the ssh2::Session struct is no longer implementing the Send trait.

Changing the std::rc::Rc to a std::sync::Arc should solve the Problem without a huge performance impact. The Change would need to be made here:
https://github.com/alexcrichton/ssh2-rs/blob/master/src/session.rs#L79

[usbpc]

It actually is more complicated to make ssh2::Session Send, I've tried to implement my proposed solution and two other structs that are used within SessionInner are not Thread safe in the way I need (and used Session before).

I'm getting these errors when I try to wrap my modified version of ssh2::session in a Mutex. I've also modifed all the places that expected a Rc to compile. My modified version can be found here: https://github.com/usbpc/ssh2-rs/tree/fix_session_not_send. I added a submodule that recreates my problems in the same repo aswell: https://github.com/usbpc/ssh2-rs/tree/fix_session_not_send/session_send. Building that results in the following errors:

error[E0277]: `*mut libssh2_sys::LIBSSH2_SESSION` cannot be shared between threads safely
 --> src/main.rs:5:1
  |
5 | / lazy_static! {
6 | |     static ref TEST: Mutex<Session> = Mutex::new(Session::new());
7 | | }
  | |_^ `*mut libssh2_sys::LIBSSH2_SESSION` cannot be shared between threads safely
  |
  = help: within `ssh2::session::SessionInner`, the trait `std::marker::Sync` is not implemented for `*mut libssh2_sys::LIBSSH2_SESSION`
  = note: required because it appears within the type `ssh2::session::SessionInner`
  = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<ssh2::session::SessionInner>`
  = note: required because it appears within the type `ssh2::session::Session`
  = note: required because of the requirements on the impl of `std::marker::Sync` for `std::sync::Mutex<ssh2::session::Session>`
  = note: required by `lazy_static::lazy::Lazy`
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

and

error[E0277]: `std::cell::RefCell<std::option::Option<std::net::TcpStream>>` cannot be shared between threads safely
 --> src/main.rs:5:1
  |
5 | / lazy_static! {
6 | |     static ref TEST: Mutex<Session> = Mutex::new(Session::new());
7 | | }
  | |_^ `std::cell::RefCell<std::option::Option<std::net::TcpStream>>` cannot be shared between threads safely
  |
  = help: within `ssh2::session::SessionInner`, the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<std::option::Option<std::net::TcpStream>>`
  = note: required because it appears within the type `ssh2::session::SessionInner`
  = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<ssh2::session::SessionInner>`
  = note: required because it appears within the type `ssh2::session::Session`
  = note: required because of the requirements on the impl of `std::marker::Sync` for `std::sync::Mutex<ssh2::session::Session>`
  = note: required by `lazy_static::lazy::Lazy`
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

[wez]

I overlooked that we used to add unsafe impl Send for Session {} when I adjusted this on 0.4; I had somehow fixated on the *mut libssh2_sys::LIBSSH2_SESSION error while I was refactoring and just assumed that was how it was before.

This is how I'm "dealing" with this in one of my applications:
https://github.com/wez/wezterm/blob/master/pty/src/ssh.rs#L16-L49

Would you mind sharing a bit more about how you'd like to use ssh2 here? From the example you linked you have a global protected by a Mutex; is that actually how you'd like to use this crate, or just something you're trying to get things to work?

The reason I ask is because I think that it might make sense to offer a couple of different submodules/structs that wrap the library primitives in a more task-focused way; by knowing more about the use cases, we can expose the right API surfaces more egonomically and deal with some of the thread safety concerns internally.

[usbpc]

My use case is probably very unusual, the reason for that beeing that I'm farily new to rust and also have to interact with a C application (Zabbix) that has some strange ways to deal with things.

I don't actually deal with multithreading in the application, instead only multiprocessing exists. But since I'm only writing a lodable module for Zabbix I just get called from it and don't have any good way to keep state across beeing called other than globals.

So what I'm doing is putting the Session and the associated TcpStream into a struct and then putting that struckt into a Mutex protected HashMap. This was the first way I figured out how to make the rust compiler happy after I understood how my lodable module is actually called by Zabbix in multiple Processes.

What would really be nice would be having a Struct already provided that owns both the TcpStream and the Session because I feel that would make moving around the Sessions a lot easier. Because the first way I wanted to implement my module was to have an evmap that could be accessed through a global to keep ssh connections alive and not start a new connection every few seconds. That all fell apart as zabbix dosen't use threads but processes. In both my current use case and the original idea not having to deal with a TcpStream and a Session would make things way easier.

Edit: I just looked over 0.4.0 some more, and Session now actually takes ownership over the TcpStream.

Edit 2: Some documentation dosen't reflect that change: https://github.com/alexcrichton/ssh2-rs/blob/master/src/session.rs#L93

[wez]

Thanks for the info on your use case!

Since I'm traveling at the moment I thought it would be best for the sake of expediency to just make this Send again, so I've pushed 0.5.0 to crates.io with that change.

[usbpc]

Thanks for the fast response!

I'm wondering tho why it is safe to tell the compiler that Session it is Send as std::cell::RefCell<std::option::Option<std::net::TcpStream>> is not Sync. According to the documentation of Arc for Arc<T> to be Send T needs to be Send and Sync.

[richardwhiuk]

@usbpc https://doc.rust-lang.org/std/net/struct.TcpStream.html is Send and Sync.

@wez Any chance of a similar change for Channel? Or is that not safe?

I'm writing an SSH client which has multiple threads, and I can't pass the different channels to different threads, to handle read and writing TCP connections:

*mut libssh2_sys::LIBSSH2_SESSION` cannot be shared between threads safely
    = help: within `ssh2::session::SessionInner`, the trait `std::marker::Sync` is not implemented for `*mut libssh2_sys::LIBSSH2_SESSION`
    = note: required because it appears within the type `ssh2::session::SessionInner`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<ssh2::session::SessionInner>`
    = note: required because it appears within the type `ssh2::channel::Channel`

[richardwhiuk]

Oh, and Listener?

[wez]

It's not automatically safe to add an unsafe impl Send for Channel {}; you can make multiple channels associated with the same session. They all must be accessed from only one thread at a time. It's also not just a matter of throwing a Mutex around some shared inner state because the exposed API surface is a bit too broad to guarantee safety in every use case.

What I'd suggest for now is that you wrap things up in a struct of your own and tell the compiler that your use case is safe by doing something like this:

https://github.com/wez/wezterm/blob/master/pty/src/ssh.rs#L45-L49

[richardwhiuk]

Can't you create this unsafety by passing the Session into a thread (as Session is Send), creating a Channel, and then repeating?

_{Sent with GitHawk}

[ubnt-intrepid]

RefCell<T> is !Sync regardless of the thread safety of T, and hence it is inappropriate to expect that Arc<SessionInner>, which wraps RefCell<TcpStream> and Session satisfies the Send bound.

What we really need to do here is one of the two things:

• Replace RefCell<TcpStream> with Mutex<TcpStream>. In this case, the additional overhead for acquiring the lock will be added to each I/O operation.
• Extract the field tcp from SessionInner and move it into the member of Session. In this case, tcp is not shared and adding internal mutability with RefCell is not needed.

In the later case, the definition of Session and SessionInner should be modified as follows:

 pub(crate) struct SessionInner {
     pub(crate) raw: *mut raw::LIBSSH2_SESSION,
-    tcp: RefCell<Option<TcpStream>>,
 }

 unsafe impl Send for SessionInner {}

 pub struct Session {
-    inner: Arc<SessionInner>,
+    inner: Arc<Mutex<SessionInner>>,
+    tcp: Option<TcpStream>,
 }

-unsafe impl Send for Session {}

Here, the reason for wrapping SessionInner in Mutex is that it (or *mut LIBSSH2_SESSION) is not guaranteed to share between multiple threads and the global lock is required.

[wez]

We're now Send + Sync for everything in 0.7.0 which I released to crates.io just now