-
Notifications
You must be signed in to change notification settings - Fork 442
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(@libp2p/tcp): race condition in onSocket #2763
base: main
Are you sure you want to change the base?
Conversation
This fixes a race condition in the onSocket listener method. onSocket gets a net.Socket parameter that needs to be closed later before the tcp server can close. The net.Socket is used to create a MultiaddrConnection but the connection is not tracked with this.connections until after it has been upgraded. If the tcp listener.close is called while a the MultiaddrConnection is waiting to be upgraded, then the MultiaddrConnection socket cannot be closed as it does not exist in this.connections. Instead of adding the MultiaddrConnection to this.connections on creation I decided to create a separate property named this.maConnections. I decided to track the non-upgraded connections separately because I was unsure how this.connections must be used with other things like metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is necessary. When we close the transport and call this.pause(true)
we close the socket server which should destroy all open sockets.
This is the same as calling maConn.abort(err)
- the interesting thing it does is call socket.destroy()
- see toMultiaddrConnection
for this.
It's possible there's something missing though - could you please add a test to show the problem you're seeing?
Actually server.close says:
So yes, it is necessary to close un-upgraded connections separately, but we should probably just close them immediately if they fail to upgrade, if we're not doing that already. |
Yes, they are aborted inside of the catch block of the upgrade call currently. The problem is that maConn is added to this.connections inside of the then block of the upgrade. If close is called before the then block is ran then the connection cannot be closed and this.server.close never resolves. |
I can add a test. The test that I have been using exists in https://github.com/tabcat/delay5 which I should be able to replicate in a separate PR. |
Title
fix(@libp2p/tcp): race condition in onSocket
Description
This fixes a race condition in the onSocket listener method.
onSocket gets a net.Socket parameter that needs to be closed later
before the tcp server can close.
The net.Socket is used to create a MultiaddrConnection but the
connection is not tracked with this.connections until after it has been
upgraded.
If the tcp listener.close is called while a the MultiaddrConnection is waiting
to be upgraded, then the MultiaddrConnection socket cannot be closed as
it does not exist in this.connections.
Instead of adding the MultiaddrConnection to this.connections on
creation I decided to create a separate property named
this.maConnections. I decided to track the non-upgraded connections
separately because I was unsure how this.connections must be used with other things
like metrics.
Notes & open questions
Closes #2760
Change checklist