Trust quorum: make some private methods synchronous #9590
Conversation
I was reading around bits of this code because it's on the scene for the hang in #9569. "Awaiting within `select!` arms" is one of our red flags for futurelock, and in reading I realized most of these arms were calling methods tagged as `async` but that didn't actually `.await` anything themselves. (There's one exception, noted below.) I think making these synchronous makes the overall `select!` structure clearer and safer; we no longer have any nested `.await`s.
| Err(e) => { | ||
| let _ = self.writer.shutdown().await; | ||
| // We need to shut down the writer - returning an error here | ||
| // will cause our caller (`run()`) to stop, which will do so. |
There was a problem hiding this comment.
This is the one method that was actually async, but I think it doesn't need to be: if we return an error here, the next thing run() does is call self.close().await which itself shuts down the writer.
If this is too action-at-a-distance I'm happy to revert this one; we can still trim down the other arms freely.
There was a problem hiding this comment.
I'm all for this change. Thank you!
| } | ||
|
|
||
| async fn ping(&mut self) -> Result<(), ConnErr> { | ||
| fn ping(&mut self) -> Result<(), ConnErr> { |
There was a problem hiding this comment.
I considered renaming this enqueue_ping(), since it's not actually performing the ping itself. Tiny clarity win, maybe?
There was a problem hiding this comment.
Strong objection to this one. No methods actually send messages really. They just enqueue data. But even syscalls just put stuff in a buffer to be processed eventually.
I'm a no on extraneous words for this reason.
andrewjstone
left a comment
andrewjstone
left a comment
There was a problem hiding this comment.
Thanks for the fix!
| } | ||
|
|
||
| async fn ping(&mut self) -> Result<(), ConnErr> { | ||
| fn ping(&mut self) -> Result<(), ConnErr> { |
There was a problem hiding this comment.
Strong objection to this one. No methods actually send messages really. They just enqueue data. But even syscalls just put stuff in a buffer to be processed eventually.
I'm a no on extraneous words for this reason.
| Err(e) => { | ||
| let _ = self.writer.shutdown().await; | ||
| // We need to shut down the writer - returning an error here | ||
| // will cause our caller (`run()`) to stop, which will do so. |
There was a problem hiding this comment.
I'm all for this change. Thank you!
I was reading around bits of this code because it's on the scene for the hang in #9569. "Awaiting within
select!arms" is one of our red flags for futurelock, and in reading I realized most of these arms were calling methods tagged asasyncbut that didn't actually.awaitanything themselves. (There's one exception, noted below.)I think making these synchronous makes the overall
select!structure clearer and safer; we no longer have any nested.awaits.