Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Sync to trait object #5

Closed
wants to merge 1 commit into from

Conversation

spacemeowx2
Copy link
Contributor

Added because all ciphers could be Sync.

Because the current Cipher is !Sync, I can not use it in my library : )

@LuoZijun
Copy link
Collaborator

@spacemeowx2 这个数据结构内部没有使用锁,所以无法在不同线程当中传递。所以,如果你需要在多线程环境里面都能访问和使用同一个 Cipher,那么你应该使用 Arc<Cipher> :)

@spacemeowx2
Copy link
Contributor Author

spacemeowx2 commented Mar 12, 2021

@LuoZijun

我认为这个PR的修改只要没有用到 unsafe, Rust编译器的检查就足矣证明这个结构是线程安全的.

如果是因为 crypto2 实现不是 Sync 的, 那么请先修正 crypto2 的结构体, 为其加上 !Sync.

附加文档链接:

https://doc.rust-lang.org/std/marker/trait.Sync.html

The precise definition is: a type T is Sync if and only if &T is Send. In other words, if there is no possibility of undefined behavior (including data races) when passing &T references between threads.

@LuoZijun
Copy link
Collaborator

LuoZijun commented Mar 12, 2021

@spacemeowx2 Cipher 内部有状态数据,这些状态数据并没有使用锁来保护,如果实现 Sync 会导致问题在多线程环境下出现问题。
至于 Crypto2 库里面的 cipher 数据结构实现了 SendSync,是因为他们内部并无状态,同时,也没有 行为,所以,这些数据在不同线程之间可以安全的传递,同时,他们的引用可以在不同线程之间使用。

所以,总结来说,Crypto2::cipher 会自动实现了 SyncSend. 但是 ss-crypto::Cipher 是实现 Send,而没有 Sync.

补充:

ss-crypto::Cipher 当然也可以实现成支持 Sync 的,那么这意味着需要在内部使用 RwLock 之类的锁机制。很显然,这种场景不是我们的主流,同时我们也无这种需求。如果你有的话,只需要套上 Arc<RwLock<Cipher>> 即可达到一样的效果。

@spacemeowx2
Copy link
Contributor Author

spacemeowx2 commented Mar 12, 2021

我认为只要Cipher所有有关写的方法是&mut借用的,就应该安全的实现Sync。Sync并不会让结构体在多个线程有多个可写借用。根据Sync的文档,只要&Cipher是Send的就符合Sync的定义。

如果我错了请指出

@LuoZijun
Copy link
Collaborator

@spacemeowx2

Sync 并不会让结构体在多个线程有多个可写借用。

没错,Sync 在定义上,只要其余的线程访问它的只读引用不会导致数据竞争就行。

我们的 Cipher 数据结构在不同线程里面访问它的只读引用,也不会导致数据竞争问题,确定地说是我们没有这个需求,我们的开放的只读函数(fn(&self)),并不涉及 NONCE 数据的读取,自然不存在竞争问题。

但是像一些标准库里面的 Receiver 之类的类型就不会允许。


我认为只要 Cipher 所有有关写的方法是 &mut 借用的,就应该安全的实现 Sync。

实现 Sync 的确安全,因为只读数据当中,本身就是安全的。但是如我上面所讲,目前用到的项目没有这个需求。如果你的需求是仅仅只用到 fn category(&self)fn tag_len(&self)fn kind(&self) 这些方法,那么的确如此,可当前的项目需求并不是如此。如果你用到了 fn encrypt_packet(&mut self, ...) 这些,必然就碰到了数据竞争问题(在多线程环境下),那么,最终,有没有实现 Sync 对于项目来讲根本不重要。也许你可以分享你们的使用场景来说明它的不可或缺性?

@spacemeowx2
Copy link
Contributor Author

spacemeowx2 commented Mar 12, 2021

在我的场景下,需要把 shadowsocks 库的 ProxyClientStream 包装在一个结构体内,这个结构体会实现一个 async-trait。而这个 trait 会被动态分发(dyn MyTrait)。因为 async-trait 实现的问题,动态分发后再使用要求 Self: Sync。这个结构体中唯一 !Sync 的就是这个 Box。RwLock并不能为类型增加Sync。

如果您觉得加 Sync 实在不妥,可以关闭这个PR,我在库里继续使用 unsafe impl 实现来workaround。

@zonyitoo
Copy link
Collaborator

@LuoZijun 实现Sync并不需要加锁,即使我们都知道Cipher的内部状态会变化,但是按照官方对于Sync的定义:

https://doc.rust-lang.org/std/marker/trait.Sync.html

The precise definition is: a type T is Sync if and only if &T is Send. In other words, if there is no possibility of undefined behavior (including data races) when passing &T references between threads.

&Cipher确实就可Send的,那么确实满足Sync的条件。

因为encrypt_packetdecrypt_packet要求&mut Self,所以并不会导致多线程同时安全调用这个函数的可能性,因为只可能有一个可变引用。

因此实现Sync是完全安全而且没有风险的,相信编译器的判断。这里并不需要增加unsafe impl Sync,从文档可得:

This trait is automatically implemented when the compiler determines it's appropriate.

我建议支持合并。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants