Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix listening on a port after it was closed not working #1531

Closed
wants to merge 2 commits into from

Conversation

t4lz
Copy link
Member

@t4lz t4lz commented Jun 10, 2023

closes #1526

The bug is that when the app closes a socket it listens to, we don't remove it from our set of Listen objects, and if it then listens again on the same port, we don't update the Listen object, which results in the local port (the port mirrord connects to) not to be updated, so mirrord tries to connect to the local port from the first time the app listened on the same requested port.

The quick fix is to replace the Listen object if the app listens again. But maybe we want to handle socket closes (remove requested port from ports set, notify agent so that the agent stops listening...).

@t4lz t4lz assigned t4lz and unassigned t4lz Jun 10, 2023
@aviramha
Copy link
Member

@meowjesty just changed it in PR #1123 that we won't let user double bind - doesn't this (re)introduce the weird side effect? (from the code change hard to tell)
I think the more proper solution would be notify remote on close, since we already hook that?

@t4lz
Copy link
Member Author

t4lz commented Jun 10, 2023

@meowjesty just changed it in PR #1123 that we won't let user double bind - doesn't this (re)introduce the weird side effect? (from the code change hard to tell) I think the more proper solution would be notify remote on close, since we already hook that?

This does not reintroduce that bug because we still make sure the socket is not bound and that there is no other bound socket with the same requested address.

Yes, I agree that the proper solution is to notify the TCP handler about the close, and I opened an issue for that.

The value of this PR would be the regression test and the temporary fix (the fix is only 2 lines). I can instead add the proper close handling in this PR if you're fine with that, or if it's going to be prioritized soon we can close this PR.

@t4lz
Copy link
Member Author

t4lz commented Jun 10, 2023

Sorry wrong issue links in my comments, fixing

@aviramha
Copy link
Member

okay I understand, sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mirrord fails on incoming traffic when running a react app
2 participants