mcginty / snow

A Rust implementation of the Noise Protocol Framework
Apache License 2.0
884 stars 120 forks source link

better multithreading story #15

Open mcginty opened 6 years ago

mcginty commented 6 years ago

Transport states can't be shared between threads, so multithreaded crypto isn't possible with the current design.

One possible outcome is to add an into_async_transport_mode() alongside the existing into_transport_mode(), which will convert to an AsyncTransportMode that is Send + Sync, and can be cloned cheaply for use across threads.

Feedback welcome.

llebout commented 5 years ago

This would be necessary for a project I am working on, what exactly are the implications of running encryption and decryption in parallel with the Noise Framework? Is there states that make each message depend on the previous one, so that two messages can't be written (encrypted) in parallel? Or you would make the call to read_message and write_message asynchronous that add to a queue and that queue is processed sequentially?

mcginty commented 5 years ago

The normal Transport maintains a counter that must be atomically increased such that a counter isn't reused for two messages, which would be catastrophic. The StatelessTransport mode, however, delegates the responsibility of counters to the application, making it a prime candidate for being parallel-friendly.

I think the next step in snow's case is to make the stateless transport mode Send + Sync, or clone-able such that it can be shared across threads without locks.

llebout commented 5 years ago

Here how the libp2p crate does it: https://github.com/libp2p/rust-libp2p/blob/master/protocols/noise/src/io.rs https://github.com/libp2p/rust-libp2p/blob/master/protocols/noise/tests/smoke.rs

They implemented several state machines and it seems like it did not bother implementation, I'm not sure about the specifics, it looks like they did not require Sync because not needing it.

They can't Read and Write in parallel though, which limits the potential of having trivial asynchronous code with async/await syntax. They always need to offload to some sort of I/O task that empties a queue that's filled by some other code.

fogti commented 4 years ago

Is StatelessTransportState currently thread safe?

mcginty commented 4 years ago

@zserik StatelessTransportState is not clone-able, so must currently live within an Arc<Mutex<StatelessTransportState>> for multi-threading workloads, which is of course not ideal since there's nothing theoretically limiting it from being the type of thread-safe we want - snow just needs a refactor.

fogti commented 4 years ago

As long as no rekeying is required, Arc<StatelessTransportState> should be sufficient. If rekeying is required, RwLock<Arc<StatelessTransportState>> might be useful, too. I currently think about writing some library which can temporarily split a connection into a ReadHalf and a WriteHalf, which should again be merged for rekeying and rehandshaking. (In that rekeying/rehs case, the working parts of the program, e.g. the read and write handlers would be temporarily suspended, which is a bit harder when integrated into async code, but maybe I find some elegant solution...)

mcginty commented 4 years ago

Yes excuse me, thank you for the correction!