Soundness concerns #46
Replies: 1 comment 1 reply
-
I'm still mastering Rust, and the lifetimes + sharing part is one of the toughest so far. That said, it's my understanding that there's absolutely no way of sharing a router instance by reference between threads without either wrapping it in Arc or using unsafe. You are right that technically router does not have to be thread-safe. Currently, however, the router is passed by a mutable raw pointer because of the The problem though is not the (im)mutability and thread safety, but liveliness. There seem to be no way to come up with the right combination of filetimes for It seems like the only way to deal with this safely is to wrap the router in Arc. Which is definitely going to hurt performance, but does it really matter? It might for simple synthetic benchmarks, but any real-world web API is very likely going to be bound by the latency of the database or queue or some third-party services. @seanpianka I agree that it would be better and safer to resurrect #25 despite the performance drop. @rousan what do you think? |
Beta Was this translation helpful? Give feedback.
-
Reposting my concerns from #45:
How should the caller guarantee the lifetimes? The RequestService can reach a Router, but the lifetime is not tied to that of the Router. This could easily cause a use-after-free or stack-use-after-return.
Similarly, does unsafe impl Send + unsafe impl Sync add data races? I do not understand how this is sound, since the Router does not have to be thread-safe.
Also, I asked a while ago about wrapping the
Router
in anArc
, and @rousan mentioned there are performances issues:The confusion here is that the reference count is incremented when you deference the Arc. This is not true -- you only increment the reference count when the Arc is cloned. When you deference the Arc, you're only checking a (thread-)local, or don't check it all (I forgot the specifics).
Unless there was a test done which showed a degradation of performance using
Arc
vsmut*
, I'd like to discuss making this change.Beta Was this translation helpful? Give feedback.
All reactions