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: internal sACK flag not set correctly for client socket #995

Merged
merged 2 commits into from
Sep 22, 2024

Conversation

XOR-op
Copy link
Contributor

@XOR-op XOR-op commented Sep 22, 2024

Bug Description

Selective ACK is used for TCP retransmission. In the current implementation, sACK option will be enabled by default for a client TCP socket. However, the remote_has_sack is not correctly set when the server sends a SYN-ACK packet back. Since remote_has_sack is false by default, client TCP sockets will not send sACK when packet loss happens.

However, when the sACK-permitted option is set in the initial SYN packet, the server will (typically) only expect sACK for fast retransmission. Since no sACK is received, the server (as a sender) will wait until retransmission timer expires. This will trigger congestion control, and users will observe an inconsistent download speed even if the network link is relatively stable.

Solution

Set remote_has_sack correctly in SynSent state. Listen state is not affected by this bug.

@Dirbaio
Copy link
Member

Dirbaio commented Sep 22, 2024

can you add a test?

@XOR-op
Copy link
Contributor Author

XOR-op commented Sep 22, 2024

@Dirbaio Corresponding test has been added.

@whitequark whitequark added this pull request to the merge queue Sep 22, 2024
Merged via the queue into smoltcp-rs:main with commit 54905ee Sep 22, 2024
9 checks passed
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.

3 participants