Skip to content

Don't stall when already delivered packets are queued#2667

Merged
robgjansen merged 1 commit intoshadow:mainfrom
robgjansen:tcp-rcv-bug
Jan 16, 2023
Merged

Don't stall when already delivered packets are queued#2667
robgjansen merged 1 commit intoshadow:mainfrom
robgjansen:tcp-rcv-bug

Conversation

@robgjansen
Copy link
Copy Markdown
Member

@robgjansen robgjansen commented Jan 13, 2023

On slow connections, it is possible that retransmitted packets end up in the input queue multiple times. Before this change, Shadow would stall because it was looking for the nth packet to deliver to the plugin, but the next in-order packet in the queue was the n-1th packet.

@robgjansen robgjansen added the Type: Bug Error or flaw producing unexpected results label Jan 13, 2023
@robgjansen robgjansen self-assigned this Jan 13, 2023
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Jan 13, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 13, 2023 •
edited
Loading

Codecov Report

Base: 68.15% // Head: 67.43% // Decreases project coverage by -0.71% ⚠️

Coverage data is based on head (252f633) compared to base (b38ad75).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2667      +/-   ##
==========================================
- Coverage   68.15%   67.43%   -0.72%     
==========================================
  Files         202      202              
  Lines       30289    30289              
  Branches     5904     5904              
==========================================
- Hits        20642    20426     -216     
- Misses       4972     5221     +249     
+ Partials     4675     4642      -33
Flag Coverage Δ
tests 67.43% <ø> (-0.72%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/host/syscall/handler/eventfd.rs 0.00% <0.00%> (-72.23%) ⬇️
src/main/host/descriptor/eventfd.rs 0.00% <0.00%> (-65.66%) ⬇️
src/main/host/syscall/handler/ioctl.rs 40.00% <0.00%> (-17.15%) ⬇️
src/main/host/syscall/handler/unistd.rs 57.31% <0.00%> (-15.02%) ⬇️
src/main/host/syscall/handler/fcntl.rs 50.00% <0.00%> (-14.29%) ⬇️
src/main/host/descriptor/descriptor_table.rs 69.56% <0.00%> (-10.15%) ⬇️
src/main/host/syscall/handler/time.rs 62.79% <0.00%> (-4.66%) ⬇️
src/main/host/syscall/handler/mod.rs 76.06% <0.00%> (-4.28%) ⬇️
src/main/host/syscall/type_formatting.rs 75.00% <0.00%> (-2.59%) ⬇️
src/main/host/memory_manager/memory_mapper.rs 69.15% <0.00%> (-2.28%) ⬇️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

stevenengler
stevenengler approved these changes Jan 15, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me! This definitely looks like the bug could cause some issues. The only thing that looks a little worrying is the transfer time graph from the benchmark, but I'm not sure what could be causing that.

Edit: Deleted two misattributed graphs.

stevenengler
stevenengler reviewed Jan 15, 2023
View reviewed changes
@robgjansen
Copy link
Copy Markdown
Member Author

robgjansen commented Jan 15, 2023

@stevenengler note that the graphs you posted are for #2632, not for this PR. (The graphs you posted are from a branch that does not yet contain this fix.)

I highly doubt that this PR will negatively affect performance, but could run a quick tgen to double check.

@robgjansen
Copy link
Copy Markdown
Member Author

robgjansen commented Jan 16, 2023

TGen benchmark confirms no change in performance: https://github.com/shadow/benchmark-results/tree/master/tgen/2023-01-15-T23-30-47

@robgjansen robgjansen merged commit 5dc806e into shadow:main Jan 16, 2023
@robgjansen robgjansen deleted the tcp-rcv-bug branch January 16, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevenengler stevenengler stevenengler approved these changes

Assignees

@robgjansen robgjansen

Labels

Component: Main Composing the core Shadow executable Type: Bug Error or flaw producing unexpected results

Projects

None yet

Milestone

Improve modularity of router object

Development

Successfully merging this pull request may close these issues.

2 participants

@robgjansen @stevenengler