[Lldb-commits] [lldb] [lldb] Add a callback version of TCPSocket::Accept (PR #106955)

[Dmitry Vasilyev via lldb-commits]

slydiman wrote:

@labath 
> Basically, keep the socket code inside the socket class, but make it asynchronous. This way you can listen for as many sockets (and other things) as you like. It shouldn't be necessary to do the socket handling stuff externally.
> 
> I haven't looked into exactly how this would fit into the Acceptor class in lldb-server, but my preferred solutions would be:
> 
> * delete it (or at least significantly slim down it's functionality -- it's a very thin wrapper over the Socket class, and probably only exists due to historic reasons)
> * make it callback-based as well
>
> I don't like the idea of making Acceptor responsible for multiple ports for the same reason I did not want to do that for TCPSocket -- it's complicated and inflexible.

I don't see any profit of this change. Using an extrernal MainLoop we can break the blocking Accept() from an other thread. But we can break it making a dummy connection too. 

The main reason to change TCPSocket was to be able to wait for multiple connections from different ports in the same blocking Accept() in the single thread.
Currently #104238 contains the best implementation of Acceptor.cpp w/o changing TCPSocket.

sock_cb parameter is useles here because it will be always the same. 
TCPSocket::m_listen_sockets is still here and it is still inaccesible externally. The method Listen() which initializes m_listen_sockets is unchanged. 
A callback may be useful (in theory) in Listen() to add a custom listen sockets to m_listen_sockets. Probably it is better to just initialize an own listen_sockets and just provide it to TCPSocket. But where this code will be placed? Acceptor.cpp, lldb-platform.cpp?

If you want to delete Acceptor, the best approach is the following:

Use Listen() from #104797 but w/o changing the connection string:
Status TCPSocket::Listen(llvm::StringRef name, int backlog, std::set<uint16_t> * extra_ports = nullptr)

The accept MainLoop must be a member of TCPSocket and it is enough to just add 
TCPSocket::BreakAccept() { m_accept_loop.AddPendingCallback([](MainLoopBase &loop) { loop.RequestTermination(); }); } 

> I haven't looked into exactly how this would fit into the Acceptor class in lldb-server

This is the problem. I don't see how this PR will help to implement accepting for multiple connections from 2 ports. It only complicates TCPSocket.

I'm trying to offer the `completed` solution which has been tested.

BTW, please approve NFC #106640.

https://github.com/llvm/llvm-project/pull/106955