UnsolicitedResponse isn't Send

[jonhoo]

As reported in https://github.com/jonhoo/rust-imap/issues/250, the UnsolicitedResponse type isn't Send, but it should be. We should fix that.

[dequbed]

Are you sure? rustdoc (and static_assertions) do show an Auto Trait implementation for Send and Sync on imap::types::UnsolicitedResponse.

Connection<T>, Client<T> and Session<T> do all also implement Send as long as T does, however they are !Sync. However that's not because of UnsolicitedResponse but because std::mpsc::Sender and std::mpsc::Receiver are always !Sync

[jonhoo]

Huh, that's very weird indeed. In that case I don't know what was causing #250. There, @soywod claimed that mpsc::Sender<UnsolicitedResponse> and mpsc::Receiver<UnsolicitedResponse> weren't thread-safe, but they should be if the underlying type is Send (which it apparently is). @soywod can you speak more to how exactly the compiler was complaining at you?

[soywod]

Here the error I get:

error[E0277]: `std::sync::mpsc::Sender<UnsolicitedResponse>` cannot be shared between threads safely
   --> src/backend/imap/backend.rs:404:10
    |
404 | impl<'a> Backend for ImapBackend<'a> {
    |          ^^^^^^^ `std::sync::mpsc::Sender<UnsolicitedResponse>` cannot be shared between threads safely
    |
    = help: within `backend::imap::backend::ImapBackend<'a>`, the trait `Sync` is not implemented for `std::sync::mpsc::Sender<UnsolicitedResponse>`
    = note: required because it appears within the type `Session<ImapSessionStream>`

When I said "not thread-safe" I meant not Sync nor Send, and indeed std::mpsc::Sender is not Sync by definition.

[MTRNord]

Note that std::sync::mpsc::Receiver isn't Sync, either. So in a std::sync::RwLock type like used by relm4 (higher level gtk lib) this causes further issues.

[MTRNord]

Fixing the sender on the other hand would be possible with https://doc.rust-lang.org/std/sync/mpsc/fn.sync_channel.html however the same error would appear for the Receiver then

[dequbed]

Fixing the sender on the other hand would be possible with https://doc.rust-lang.org/std/sync/mpsc/fn.sync_channel.html however the same error would appear for the Receiver then

Hmm, as I understand the code the unsolicited_responses is really mostly used in a 'self-pipe' pattern, with Session always holding both the sending and receiving end and in essence using it as a FIFO queue. Especially given that right now the uses of unsolicited_responses_tx always take it as a &mut anyway, I do think it would make sense to use a VecDeque or plain Vec instead. That would also enable the multi-threaded access patterns that I think @soywod wants to work with.

Your thoughts @jonhoo? I'd be happy to write a PR for that change if you agree that it makes sense.

[soywod]

That would also enable the multi-threaded access patterns that I think @soywod wants to work with.

I am really interested in this solution, because the actual implementation I have (using a connection pool) seems to suffer from perfs and mutex lock issues.

I'd be happy to write a PR for that change if you agree that it makes sense.

Please let me know if I can do anything to help also!

[jonhoo]

Ah, so the concern is actually that the client isn't Send/Sync (because it holds these senders/receivers), not that the elements of the channel aren't. That does make more sense! Yes, I think replacing it with a VecDeque makes a lot of sense, and would be happy to look at a PR!

[soywod]

I initiated a PR, please let me know if it resembles what you had in mind.

[soywod]

With the PR, I do not have anymore the cannot be shared between threads safely, but I still cannot run multiple tasks in parallel using a same Session. My intuition is that every session fn borrows self as mut &self mut which cannot be shared safely between threads without a Mutex. The PR has no impact on the initial problem.

[jonhoo]

Ah, yes, so, that's sort of fundamental to the IMAP protocol. It doesn't allow for multiple concurrent requests. So you'd need to either use a Mutex or spin up a separate Session for each client unfortunately.