-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: deadlock when sending ping #193
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -391,14 +391,6 @@ impl<T: AsyncRead + AsyncWrite + Unpin> Active<T> { | |
fn poll(&mut self, cx: &mut Context<'_>) -> Poll<Result<Stream>> { | ||
loop { | ||
if self.socket.poll_ready_unpin(cx).is_ready() { | ||
// Note `next_ping` does not register a waker and thus if not called regularly (idle | ||
// connection) no ping is sent. This is deliberate as an idle connection does not | ||
// need RTT measurements to increase its stream receive window. | ||
if let Some(frame) = self.rtt.next_ping() { | ||
self.socket.start_send_unpin(frame.into())?; | ||
continue; | ||
} | ||
|
||
// Privilege pending `Pong` and `GoAway` `Frame`s | ||
// over `Frame`s from the receivers. | ||
if let Some(frame) = self | ||
|
@@ -409,6 +401,14 @@ impl<T: AsyncRead + AsyncWrite + Unpin> Active<T> { | |
self.socket.start_send_unpin(frame)?; | ||
continue; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing this order seem to fix the issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it deadlock on yamux test too? If not, maybe we could add a test for this too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please elaborate? I understand this test was designed specifically to address this type of deadlock. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually missed the test that was there so my comment can be ignored :) |
||
// Note `next_ping` does not register a waker and thus if not called regularly (idle | ||
// connection) no ping is sent. This is deliberate as an idle connection does not | ||
// need RTT measurements to increase its stream receive window. | ||
if let Some(frame) = self.rtt.next_ping() { | ||
self.socket.start_send_unpin(frame.into())?; | ||
continue; | ||
} | ||
} | ||
|
||
match self.socket.poll_flush_unpin(cx)? { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those changes seem to make the test flaky and the deadlock happens sometimes. The test output when it happens is:
Would it also be possible to achieve the same using smaller numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
I can usually reproduce it at least once when running 5 times. But not when capacity is 100. Maybe it can give some idea about the cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I override
DEFAULT_CREDIT
andPING_INTERVAL
for this test? I changed the values where they are defined as I didn't know how to override them in the test.