Skip to content

fix(TCPServer): continues to accept connections after stop() #4892#4896

Merged
aleks-f merged 5 commits intomainfrom
4892-tcp-server-accept-after-stop
Mar 12, 2025
Merged

fix(TCPServer): continues to accept connections after stop() #4892#4896
aleks-f merged 5 commits intomainfrom
4892-tcp-server-accept-after-stop

Conversation

@aleks-f
Copy link
Copy Markdown
Member

@aleks-f aleks-f commented Mar 11, 2025

No description provided.

@aleks-f aleks-f added this to the Release 1.14.2 milestone Mar 11, 2025
@aleks-f aleks-f requested review from matejk and obiltschnig March 11, 2025 14:37
@aleks-f aleks-f self-assigned this Mar 11, 2025
@aleks-f aleks-f added this to 1.14 Mar 11, 2025
@aleks-f
Copy link
Copy Markdown
Member Author

aleks-f commented Mar 11, 2025

looks like the only way to do this is to have a thread-safe sockfd wrapper, or take the other route and allow nullptr return from connection factory

@obiltschnig
Copy link
Copy Markdown
Member

In TCPServer::run(), when _socket.poll() returns true, check _stopped again before calling _socket.acceptConnection(), and if it's set, return. That should prevent any new connections from being accepted after stop() has been called.

@obiltschnig
Copy link
Copy Markdown
Member

Also, first calling TCPServer::stop() and then killing existing connections may prevent aggressive clients from reconnecting.

@aleks-f
Copy link
Copy Markdown
Member Author

aleks-f commented Mar 11, 2025

Also, first calling TCPServer::stop() and then killing existing connections may prevent aggressive clients from reconnecting.

That can be internalized - tell factory to stop creating new connections before the existing ones are terminated. We just have to allow factory to return null to the dispatcher. That way, all a user has to do is respect the factory status and return null when stopped.

@obiltschnig
Copy link
Copy Markdown
Member

If you handle this in the factory, by the time the factory returns null the connection has already been accepted and put in the dispatcher queue. Not calling accept() in the first place is preferable.

@aleks-f
Copy link
Copy Markdown
Member Author

aleks-f commented Mar 11, 2025

If you handle this in the factory, by the time the factory returns null the connection has already been accepted and put in the dispatcher queue. Not calling accept() in the first place is preferable.

I am not opposed to doing that. But doing it on accept is still not 100% bulletproof. If factory returns null, the accepted socket will die with the notification. Additionally, it gives more control to the factory, eg. it can limit the number of connections it is willing to create or refuse connections from certain clients etc.

@obiltschnig
Copy link
Copy Markdown
Member

I'm ok with allowing the TCPServerConnectionFactory return null.

@aleks-f
Copy link
Copy Markdown
Member Author

aleks-f commented Mar 12, 2025

@obiltschnig do you want me to leave the sockfd atomic? there may be some benefit from it, but it is not nearly a proper solution for socket thread safety

@aleks-f aleks-f merged commit eb94de0 into main Mar 12, 2025
47 checks passed
@aleks-f aleks-f deleted the 4892-tcp-server-accept-after-stop branch March 12, 2025 21:15
matejk pushed a commit that referenced this pull request Mar 13, 2025
…4896)

* fix(TCPServer): continues to accept connections after stop() #4892

* fix(TCPServer): first attempt to silence TSAN #4892

* fix(TCPServer): check stopped status after poll; add TCPServerFactory::stop() and allow factory to return nullptr on connection creation request #4892

* fix(TCPServer): initialize factory stopped flag #4892

* revert(SocketImpl): atomic sock fd
@matejk matejk moved this to Done in 1.14 Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants