Skip to content

TCP: Un-parent child sockets once they've been accepted#3643

Merged
sporksmith merged 5 commits intoshadow:mainfrom
sporksmith:child-sockets
Aug 27, 2025
Merged

TCP: Un-parent child sockets once they've been accepted#3643
sporksmith merged 5 commits intoshadow:mainfrom
sporksmith:child-sockets

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith commented Aug 19, 2025

Once a child socket has been accepted (associated with a file descriptor and moved off of the listening socket's queue), we now remove the parent-child relationship and associate the local:remote address pair directly with the corresponding network interface, instead of continuing to route packets to the listening socket and having the listening socket route them to the child.

This allows a listening socket to be unbound and cleaned up when it's closed instead of keeping it around to route packets to the child sockets, which allows us to reuse the address it had been bound to.

Fixes #3563.

I noticed and filed #3644 while working on this but verified that it was present before this PR as well.

@github-actions github-actions bot added Component: Testing Unit and integration tests and frameworks Component: Main Composing the core Shadow executable labels Aug 19, 2025
@sporksmith
Copy link
Copy Markdown
Contributor Author

I think we might lose data if the client sends data and then closes the connection before the server accepts the connection. This may or may not be the case before this PR.

TODO: figure out whether this happens and whether it happened before; fix it or file/update an issue.

@sporksmith
Copy link
Copy Markdown
Contributor Author

I think we might lose data if the client sends data and then closes the connection before the server accepts the connection. This may or may not be the case before this PR.

TODO: figure out whether this happens and whether it happened before; fix it or file/update an issue.

Hmm, maybe this is why the tgen tests are failing

@sporksmith
Copy link
Copy Markdown
Contributor Author

I think we might lose data if the client sends data and then closes the connection before the server accepts the connection. This may or may not be the case before this PR.

TODO: figure out whether this happens and whether it happened before; fix it or file/update an issue.

Added a test for this. It's handled correctly before and after.

Hmm, maybe this is why the tgen tests are failing

It was something else - destroying the TCP server state while while references to the child sockets can still exist elsewhere and try to access it. Fixed.

Copy link
Copy Markdown
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me other than my question about port numbers, but I think we might want @stevenengler to take a look too because of the importance of TCP correctness for us.

Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

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

LGTM! I just have a few optional comments.

I can't comment on the ->child->parent refcounting stuff, but as long as the tests pass without shadow segfaulting then it's good with me :)

On failure this lets us see which part failed.
Regression test for shadow#3563.
Currently doesn't pass under shadow.
Once a child socket has been `accept`ed (associated with a file
descriptor and moved off of the listening socket's queue), we now remove
the parent-child relationship and associate the local:remote address
pair directly with the corresponding network interface, instead of
continuing to route packets to the listening socket and having the
listening socket route them to the child.

This allows a listening socket to be completely cleaned up when it's
closed instead of keeping it around to route packets to the child
sockets, which allows us to disassociate and subsequently reuse the
listening address.

Fixes shadow#3563
@sporksmith sporksmith enabled auto-merge August 27, 2025 15:41
@sporksmith sporksmith merged commit 9fea47c into shadow:main Aug 27, 2025
24 checks passed
@sporksmith sporksmith deleted the child-sockets branch August 27, 2025 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parent TCP sockets aren't disassociated from the network interface until all child sockets are closed

3 participants