Remove async-trait and Box<dyn Stream>
#561
Conversation
| fn proposal_chunks_stream( | ||
| &self, | ||
| view: View, | ||
| ) -> impl Future<Output = impl Stream<Item = ProposalMsg> + Send + Sync + Unpin + 'static> + Send; | ||
| fn broadcast(&self, message: NetworkMessage) -> impl Future<Output = ()> + Send; | ||
| fn timeout_stream( | ||
| &self, | ||
| committee: &Committee, | ||
| view: View, | ||
| ) -> impl Future<Output = impl Stream<Item = TimeoutMsg> + Send + Sync + Unpin + 'static> + Send; | ||
| fn timeout_qc_stream( |
There was a problem hiding this comment.
Ufff, seeing this I do not think the code readability is actually improve and all...
There was a problem hiding this comment.
We can directly use async keyword in the trait here, but it will show a warning. We can use #[allow(...)] to ignore the warnings. But still, readability is not as good as BoxStream and using async_trait. For performance, we avoid two Box allocations, one for Box stream, and another for Box future.
When implementing the trait, async keyword can be directly used, but in trait definition, by default, future fn is not Send.
There was a problem hiding this comment.
We can directly use
asynckeyword in the trait here, but it will show a warning. We can use#[allow(...)]to ignore the warnings. But still, readability is not as good asBoxStreamand usingasync_trait. For performance, we avoid twoBoxallocations, one for Box stream, and another for Box future.
For now performance is not an issue. I prefer clean code that messy one. I would like to know the opinion of the team on this but we are not in a hurry to change this. I remember reading that they will be improving it (remember something about forcing code to be send or not and that there is a macro for it).
IMO for now we can stall this.
Lets vote on it @bacv @youngjoon-lee @zeegomo
🟢 Merge 🔴 Keep
There was a problem hiding this comment.
https://crates.io/crates/trait-variant is the macro that can help, but will introduce an extra useless local trait.
There was a problem hiding this comment.
https://crates.io/crates/trait-variant is the macro that can help, but will introduce an extra useless local trait.
Yes, for that is more comfortable to keep the [async_trait] for now. This is introducing complexity instead of making it simpler. IMO
Use impl async in return place to make most of code zero-cost abstraction.
This PR seems large, but there are no logic changes.