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

Mac #303

Closed
wants to merge 11 commits into from
Closed

Mac #303

wants to merge 11 commits into from

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Aug 2, 2019

edit: oops, I've meant to make PR for a different fork :)

cbranch and others added 11 commits July 19, 2019 21:32
If a socket is closed and the remote side sends a RST before the local
side sends its own RST, it is possible to jump directly from FIN-WAIT-1
to CLOSING without ever sending a RST to the remote side.

The main side effect is that if untransmitted data is present, the
socket is never exhausted, causing `EthernetInterface::poll` to loop
forever.

The ideal fix is to follow RFC more closely and not transition to
FIN-WAIT-1 until the RST has been sent, but there are too many
assumptions based on the current state that would be broken by remaining
in ESTABLISHED with a closed transmit buffer to be worth fixing
a relatively rare edge case. Instead, add another illegal transition to
fix the failures of a previous violation, in the spirit of all good TCP
stacks.
In the FIN-WAIT-2 state, we have closed the transmit side of our
connection with acknoledgement from the remote side, but we may still
receive data from the remote side. The motivating example is a simple
request/response protocol where the client sends a RST immediately
after its request before/during the server's response.
Forcing calling code to loop reduces socket processing latency for the
calling code, but this does break the currnet API as you now only
want to wait to poll again when socket readiness stops changing.
Greater than 3 sACK blocks is seen on real devices. Bumping up to
4 which is the maximum permitted in any case.
@kornelski kornelski closed this Aug 2, 2019
@whitequark
Copy link
Contributor

@kornelski Which fork?

@kornelski
Copy link
Contributor Author

@pothos
Copy link
Contributor

pothos commented Aug 4, 2019

Nice to see some work is done and even in the open. @cbranch and @kornelski, maybe you can give some context? Do you want to upstream the patches here someday or mind if others are doing it since you kept the license?

@whitequark
Copy link
Contributor

I am guessing the problem is that I don't have enough review bandwidth.

@pothos
Copy link
Contributor

pothos commented Aug 4, 2019

That's probably one factor in general but the current situation is not terribly bad and could be much worse – you did quite well ;)

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