perf/refactor: use tokio_util::sync::PollSender for ActiveSession -> SessionManager messages #4603
Conversation
|
I'm realizing now I maybe should have opened the PR directly on the repo instead of through my fork, because it didn't present me with the template flow. Sorry! Let me know if I should re-open. |
cbrit
changed the title
onbjerg
added
C-debt
| Poll::Ready(Ok(_)) => { | ||
| let _ = to_session_manager_poll_tx.send_item(msg); | ||
| } | ||
| } | ||
| Poll::Ready(Err(_)) => {} |
There was a problem hiding this comment.
the error cases mean that the channel is closed, which should not be possible but we should return Poll::Ready in that case
There was a problem hiding this comment.
Was this a bug in the previous logic? I was trying to replicate it:
TrySendError::Closed(_) => {}Maybe I'm misreading. Happy to fix just want to clarify!
| if let Poll::Ready(Ok(_)) = to_session_manager_poll_tx.poll_reserve(cx) { | ||
| let _ = to_session_manager_poll_tx.send_item( | ||
| ActiveSessionMessage::ProtocolBreach { peer_id: this.remote_peer_id }, | ||
| ); |
There was a problem hiding this comment.
This does not quite have the same effect as before,
here I think we can just set the message to pending_message_to_session, which will make sure the message gets delivered
Fixed! |
| let smtx = this.to_session_manager.inner().to_owned(); | ||
| let mut to_session_manager_poll_tx = PollSender::new(smtx); | ||
|
|
There was a problem hiding this comment.
this is an issue because this PollSender is dropped when poll returns, meaning if it's in the process of acquiring a permit it's progress gets lost,
hmm now this suddenly needs a bit more work, because to_session_manager is a MeteredSender, but PollSender requires mpsc::Sender...
so if we want to keep the metrics of MeteredSender we probably need a new helper type MeteredPollSender that mimics PollSender
does that make sense?
There was a problem hiding this comment.
Ah yes it does. Actually originally I changed MeteredSender to wrap a PollSender but opted not to because of complexity and it felt out of scope. A separate type makes more sense! I forgot to consider the ownership aspect (too much Go dev lately.. :) ).
I'll get on that.
EDIT: One question: MeteredSender counts errors, but the only error condition here is a closed connection, which can only happen once. Should I just forego the error count on the new type, or instead replace it with a count of back pressure on the channel or something?
There was a problem hiding this comment.
Updated with some assumptions about the above.
| let sender = self.to_session_manager.inner().get_ref(); | ||
| if sender.is_none() { | ||
| return Ok(()) | ||
| } | ||
|
|
||
| match sender | ||
| .unwrap() |
There was a problem hiding this comment.
style:
let's use let Some(sender) ... {else return Ok(())}
pattern here
There was a problem hiding this comment.
I am embarrassed to say I have not seen let-else before. This is wonderful.
Codecov Report
... and 9 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Meant to address #2695. There is already unit test coverage for the branches I touched; please let me know if there are any other unit tests that would be appropriate I didn't notice.
Note: I'm not the assignee for the issue, but it's been stale long enough that I went ahead.