[inetstack] Bug Fix: Simultaneous close misses FIN resend #1538
+272
−212
Conversation
This file contains hidden or 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
added
bug
|
libos = catnap
|
|
libos = catpowder
|
|
libos = catnip
|
anandbonde
reviewed
May 2, 2025
| Sender::send_ack(cb, layer3_endpoint); | ||
| }, | ||
| state => warn!("Ignoring data received after FIN (in state {:?}).", state), | ||
| if data.len() > 0 { |
There was a problem hiding this comment.
We should return early by flipping the condition. This will reduce the level of nesting in this function as well.
anandbonde
reviewed
May 2, 2025
| }; | ||
| // Store whether the packet has data here because processing it will consume the DemiBuffer. | ||
| let has_data: bool = data.len() > 0; | ||
| Self::process_data_if_any(cb, layer3_endpoint, data, seg_start, seg_end, seg_len)?; |
There was a problem hiding this comment.
Can this call be avoided by checking has_data above?
anandbonde
reviewed
May 2, 2025
| layer3_endpoint: &mut SharedLayer3Endpoint, | ||
| now: Instant, | ||
| ) -> Result<(), Fail> { | ||
| // Is this a FIN packet? |
There was a problem hiding this comment.
This comment can be removed as the next line is unambiguous.
anandbonde
reviewed
May 2, 2025
|
|
||
| pub fn get_receive_window_size(&self) -> u32 { | ||
| let bytes_unread: u32 = (self.receive_next_seq_no - self.reader_next_seq_no).into(); | ||
| // The window should be less than 1GB or 64KB without scaling. |
There was a problem hiding this comment.
It would be good to not assume these absolute sizes in the comments. These are susceptible to going stale in case the values of constants change in future.
anandbonde
reviewed
May 2, 2025
| // This inserts the segment and wakes a waiting pop coroutine. | ||
| self.pop_queue.push(temp.1); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This conditional can be inverted to reduce nesting. The indentation level is pretty high in this function. Along with that, it can be broken down into smaller functions if the indentation/nesting still remains high.
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 closes #1529 and #1537