Skip to content
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 fast retransmit #233

Closed
wants to merge 2 commits into from
Closed

Conversation

pothos
Copy link
Contributor

@pothos pothos commented Jun 12, 2018

  • Only trigger Fast Retransmit for data to send
  • Reset Retrasmission Delay after Fast Retrasmit

Small notice: The test ack_number < self.remote_last_seq is simple and used in https://github.com/google/netstack/blob/master/tcpip/transport/tcp/snd.go#L473 (reverse as ack == s.sndNxt) but maybe this does not work for sending FIN as calculated e.g. by seq_to_transmit(). Maybe more robust would be using a version of seq_to_transmit_from(ack_number) with ack_number instead of self.remote_last_seq to calculate if there is something to be sent?
Edit: Never mind, since self.local_seq_no is used to set self.remote_last_seq this calculation does not matter.

@pothos pothos mentioned this pull request Jun 12, 2018
@pothos
Copy link
Contributor Author

pothos commented Jun 12, 2018

I've updated and expanded the tests.

This fixes entering a loop when both endpoints
are stuck in retransmission.

smoltcp-rs#224
@barskern
Copy link
Contributor

barskern commented Jun 12, 2018

I think some of the calulation youre thinking about regarding seq_to_transmit is located here.

Or at least this is where the actual length of the package is calulated based on the SYN and FIN bits.

@@ -143,7 +143,7 @@ impl Timer {

fn set_for_retransmit(&mut self, timestamp: Instant) {
match *self {
Timer::Idle { .. } => {
Timer::Idle { .. } | Timer::FastRetransmit { .. } => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this decision?

If we're already doing a fast retransmit, why would we change it to a retransmit with a timeout?

Is it related to exiting from the fast retransmit state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, set_for_retransmit is called after all was send out, so this leaves the fast retransmit mode and should wait now instead of looping in poll.

@whitequark
Copy link
Contributor

@m-labs-homu r+

@m-labs-homu
Copy link

📌 Commit 277e7a3 has been approved by whitequark

m-labs-homu pushed a commit that referenced this pull request Jun 18, 2018
Closes: #233
Approved by: whitequark
@m-labs-homu
Copy link

⌛ Testing commit 277e7a3 with merge 8f3b46f...

m-labs-homu pushed a commit that referenced this pull request Jun 18, 2018
This fixes entering a loop when both endpoints
are stuck in retransmission.

#224

Closes: #233
Approved by: whitequark
@m-labs-homu
Copy link

💔 Test failed - status-travis

@whitequark
Copy link
Contributor

@m-labs-homu retry

@m-labs-homu
Copy link

⌛ Testing commit 277e7a3 with merge 7699265...

m-labs-homu pushed a commit that referenced this pull request Jun 18, 2018
This fixes entering a loop when both endpoints
are stuck in retransmission.

#224

Closes: #233
Approved by: whitequark
@m-labs-homu
Copy link

☀️ Test successful - status-travis
Approved by: whitequark
Pushing 7699265 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants