[inetstack] Implement async_close #530
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iyzhang
force-pushed
the
enhancement-inetstack-async-close
branch
from
b73b821
to
4dcf75f
Compare
ppenna
approved these changes
Mar 9, 2023
| // Remember that the user has called close. | ||
| self.user_is_done_sending.set(true); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Handle moving the connection to the closed state. | ||
| /// | ||
| /// This function runs the TCP state machine once it has either sent or received a FIN. This function is only for |
There was a problem hiding this comment.
If this function is meant for closing established connections only, shouldn't we name it accordingly, like poll_close_established()?
There was a problem hiding this comment.
I figured that was unnecessary since it is inside the established socket already.
ppenna
reviewed
Mar 9, 2023
src/rust/inetstack/mod.rs
Outdated
| FutureOperation::from(fut) | ||
| }, | ||
| Some(QType::UdpSocket) => { | ||
| let udp_op = UdpOperation::Pushto(qd, self.ipv4.udp.do_close(qd)); |
There was a problem hiding this comment.
In order to keep a symmetric design, don't we want a self.ipv4.udp.do_async_close()?
There was a problem hiding this comment.
This is symmetric with how UDP handles Push, where there is nothing asynchronous to do, so I left it without an additional implementation. (In fact, I forgot to change the name of the UdpOperation!)
iyzhang
force-pushed
the
enhancement-inetstack-async-close
branch
from
4dcf75f
to
6876763
Compare
ppenna
force-pushed
the
enhancement-inetstack-async-close
branch
from
6876763
to
78bf36b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements asynchronous close for the inet stack. Importantly, this PR creates a future/coroutine for driving the TCP connection to the close state and then freeing resources associated with the queue/socket. This PR leaves TODOs for actually implementing the TCP close state transitions for a later PR. Currently, the future immediately returns without being yielding, so there is no need to wake up the future. We will need to store a waker in the established socket, separate from the one for receiving data for close operations. Note that there is no need to free any of the socket data structures or control blocks explicitly because they are now all owned by the IoQueueTable, so when we free the queue descriptor, all associated metadata should be correctly freed as well. This means that it is important for us to not free the queue descriptor or SocketIds in the addresses backmap until we are sure that we will not continue to receive any more packets for the connection.