Convert Peerset into a shared sync struct
#13021
Conversation
dmitry-markin
added
A0-please_review
| pub async fn peer_reputation(self, peer_id: PeerId) -> Result<i32, ()> { | ||
| let (tx, rx) = oneshot::channel(); | ||
| pub fn peer_reputation(&self, peer_id: PeerId) -> impl Future<Output = Result<i32, ()>> + Send { | ||
| // TODO: refactor into sync function. |
There was a problem hiding this comment.
Missed this one, converting into draft.
| Poll::Ready(Some(event)) => event, | ||
| Poll::Ready(None) => return Poll::Pending, | ||
| }; | ||
| fn poll_next(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Option<Self::Item>> { |
There was a problem hiding this comment.
I'm not a huge fan of this design choice. Previously there was a clear difference between the protocols that would use Peerset and the protocol that owned Peerset and I think it made sense. Now it seems that any protocol that acquires a handle to Peerset can poll it and drain events from it even if it has no idea what to do with them.
There was a problem hiding this comment.
Would splitting Peerset into Peerset (lacking polling) and PeersetStream, both actually containing an Arc and different only in terms of the provided interface, make it better?
There was a problem hiding this comment.
The naming can be different, of course.
There was a problem hiding this comment.
I think splitting them into two would be good.
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
A note to not forget what is happening here. Before this PR, After removing the channel for incoming commands and making the interface for incoming commands synchronous, it became possible that the command from One possible solution would be to make the Nevertheless, it's unclear whether this would be sufficient, because |
|
Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Yes, I'm going to continue working on this. |
Btw, based on this comment, there may be some quite significant changes to the peerset at some point. |
Yeah, the race conditions @tomaka is mentioning are also in the |
|
Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Superseded by #13611. Closing this PR. |
Resolves paritytech/polkadot-sdk#529.
Follow-up
Things could be simplified further by removing
Message::AcceptandMessage::Rejectvariants fromsc_peerset::Messageand makingPeerset::incoming()return result synchronously. Unfortunately, this requires modifyingNotifications, and as agreed with @altonen and @bkchr should be done only after it's better covered by tests.