Skip to content

Improve the descriptor close process#1650

Merged
stevenengler merged 10 commits intoshadow:mainfrom
stevenengler:socket-close
Sep 20, 2021
Merged

Improve the descriptor close process#1650
stevenengler merged 10 commits intoshadow:mainfrom
stevenengler:socket-close

Conversation

@stevenengler
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler commented Sep 18, 2021

This PR makes some improvements to decouple the descriptor table and legacy descriptor objects. It should improve simulation accuracy and make it easier to write rust syscalls that need to work with legacy descriptors. It also closes all descriptors when the process ends, but doesn't disassociate them, so this appears to fix #1596.

@stevenengler stevenengler added the Type: Enhancement New functionality or improved design label Sep 18, 2021
@stevenengler stevenengler self-assigned this Sep 18, 2021
@github-actions github-actions bot added Component: Build Build/install tools and dependencies Component: Main Composing the core Shadow executable labels Sep 18, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 18, 2021

Codecov Report

Merging #1650 (e469c23) into main (a33b925) will increase coverage by 0.15%.
The diff coverage is 76.78%.

❗ Current head e469c23 differs from pull request most recent head c9e0b4a. Consider uploading reports for the commit c9e0b4a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1650      +/-   ##
==========================================
+ Coverage   56.19%   56.35%   +0.15%     
==========================================
  Files         140      140              
  Lines       18561    18595      +34     
  Branches     4471     4483      +12     
==========================================
+ Hits        10431    10479      +48     
+ Misses       5422     5391      -31     
- Partials     2708     2725      +17     
Flag Coverage Δ
tests 56.35% <76.78%> (+0.15%) ⬆️

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

Impacted Files Coverage Δ
src/main/host/process.c 67.91% <38.46%> (-1.35%) ⬇️
src/main/host/descriptor/tcp.c 77.42% <72.00%> (-0.18%) ⬇️
src/main/host/descriptor/socket.c 65.67% <75.00%> (+0.02%) ⬆️
src/main/host/descriptor/transport.c 64.70% <75.00%> (+0.42%) ⬆️
src/main/host/descriptor/descriptor.c 75.77% <82.05%> (+0.77%) ⬆️
src/main/core/scheduler/scheduler.c 74.34% <100.00%> (-0.53%) ⬇️
src/main/core/worker.c 76.49% <100.00%> (+0.57%) ⬆️
src/main/host/descriptor/channel.c 29.89% <100.00%> (+2.34%) ⬆️
src/main/host/descriptor/epoll.c 77.85% <100.00%> (+0.72%) ⬆️
src/main/host/descriptor/eventd.c 78.46% <100.00%> (+1.99%) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a33b925...c9e0b4a. Read the comment docs.

@stevenengler stevenengler force-pushed the socket-close branch 4 times, most recently from b20ef21 to 4648b99 Compare September 20, 2021 14:11
@stevenengler stevenengler marked this pull request as ready for review September 20, 2021 14:13
This is useful for breaking circular references that prevent the
objects from being freed. This is currently only used for sockets.
The weak references between the parent and child sockets now allows
them to be freed automatically.
The watch listeners have a non-counted reference to its epoll
instance, so make sure they are removed properly.
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith left a comment

Choose a reason for hiding this comment

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

Looks like a nice improvement!

@stevenengler
Copy link
Copy Markdown
Contributor Author

@sporksmith Re-adding you as reviewer since I changed up the reference counting logic, and want to make sure I didn't mess anything up.

@stevenengler stevenengler merged commit e4c319d into shadow:main Sep 20, 2021
@stevenengler stevenengler deleted the socket-close branch September 20, 2021 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Build Build/install tools and dependencies Component: Main Composing the core Shadow executable Type: Enhancement New functionality or improved design

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Repeated FIN packets without acknowledgement

2 participants