Consider making the LdapConn Send

[golddranks]

At the moment LdapConn contains Rc which prevents them moving between threads. This in turn prevents some connection pooling patterns in multi-threaded applications.

I encountered this when trying to implement a r2d2 adapter crate for ldap3:

26 | impl r2d2::ManageConnection for LdapConnectionManager {
   |      ^^^^^^^^^^^^^^^^^^^^^^ `std::rc::Rc<std::cell::RefCell<tokio_core::reactor::Core>>` cannot be sent between threads safely
   |
   = help: within `ldap3::LdapConn`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<std::cell::RefCell<tokio_core::reactor::Core>>`
   = note: required because it appears within the type `ldap3::LdapConn`
   = note: required by `r2d2::ManageConnection`

[inejge]

You'll note that Ldap is also full of Rcs, hence also not Send. The whole async layer is architected for the single-threaded case, so what you're proposing doesn't seem possible. Maybe some kind of proxied adapter could be created to enable synchronous connection pooling, but that's not something I've planned to work on.

[fdubois1]

What would be needed to be able to use the LdapConn on multi-thread ? In other words, what changes would be needed to make the LdapConn Send and Sync ? I saw lot of Rc and RefCell and even if we changed that for Arc and RwLock for example, the object Core from tokio_core::reactor also contains Rc object...

In the other thread, @inejge , you wrote :

This makes the handles unusable with multithreaded connection pools, and also, although I haven't checked, with the new-style Tokio default multithreaded executor; the library was build around the original Tokio which combined the reactor and the executor in a single thread.

I'm not enough familiar with tokio to understand deeply in the code, but does it mean that tokio has new executor that could works with multi threads ?

What I plan to do is to create one thread per connection and use channel to send/receive the command/result to those threads, but I'm wondering if I could do better even if it needs improvement in the lib.

[inejge]

As you've seen, Rc and RefCell would have to be replaced wholesale for the underlying async connection struct (Ldap), then LdapConn would need a multi-threaded runtime internally. The newer tokio (as opposed to tokio-core) has two runtimes: tokio::runtime::Runtime and tokio::runtime::current_thread::Runtime. The former is multithreaded and only accepts Send futures, while the latter will also work with non-Send ones.

My doubts about porting everything to Arc/Mutex|RwLock are wrt performance, and probably unfounded since the library involves network I/O, which would drown any atomics overhead, but I haven't tested it, and would like to before committing to the job. Abstracting Send/non-Send behind some kind of generic interface would be nice, but I believe it's very cumbersome or impossible with today's Rust.

[fdubois1]

Thank you for your answer. For now, I think I will create a new connection when it is needed and see if it is a problem. When I want to validate a username/password, I will create a connection to do a bind and see if the credential are good. Anyway, I'm not sure it is a good idea to reuse a connection to validate another user and use that same connection to do a search later since the bound user could change.

And if later it is needed, I will have a look to tokio::runtime::Runtime and see if I could change the core member in LdapConn with that runtime.

Thanks