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

[Pavel Labath via lldb-commits]

labath 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.

This wasn't my main motivation, but now that you mention it, I actually think this may be reason enough to do this. Using a dummy connection to break out of a blocking accept is.. clever, but I wouldn't consider it a good practice. I also think we have other places in lldb that could use the ability to interrupt a blocking operation, and I wouldn't want to start adding dummy connections there as well. (And I'm pretty sure I can come up with a (contrived) scenario where that dummy connect will actually fail.)

> 
> 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. 

In a single thread -- yes. In the same blocking accept -- not really, at least not for me.

> Currently #104238 contains the best implementation of Acceptor.cpp w/o changing TCPSocket.

I guess we'll have to agree to disagree on that.

> 
> sock_cb parameter is useles here because it will be always the same.

I'll have to disagree with that. The reason we have two ports is because we want to use them for different things. One of them will be launching a platform instance, the other a gdbserver instance. So, even though the logic will be roughly similar (both are spawning a new process) each will be doing something slightly different. With two sockets, you can pass one callback to the gdbserver socket and a different one to the platform socket.

But even if the callback was the same every time, I'd still think this is a good separation between "how to accept a connection" and "what to do with the accepted connection".

> TCPSocket::m_listen_sockets is still here and it is still inaccesible externally. The method Listen() which initializes m_listen_sockets is unchanged.

Yes, and there's no reason to change that. (I'd definitely like to change that, but that's not necessary here)

> 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?

Now I'm just thinking we haven't understood each other about how this could be used. There's no reason to pass additional sockets, because there's no reason to have a unified socket list (well.. there is, but it's already present in the main loop). To listen on two (or however many) ports, you just create however many TCPSocket instances you want, and then do a mainloop.Run(). And yeah, I'd ideally place the code in lldb-platform.cpp (that's the proper place for a main loop).

The code would be something like:
```
MainLoop loop;
platform_socket->Accept(loop, spawn_platform_cb);
server_socket->Accept(loop, spawn_gdbserver_cb);
loop.Run();
```
> 
> If you want to delete Acceptor, the best approach is the following:
I do, but it's not my primary goal. Definitely not at the cost of putting a MainLoop member inside TCPSocket.

If it turns out to be easier, I'd be fine with turning `platform_socket` and `server_socket` instances from the above snippet into `Acceptor`s.


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