[Lldb-commits] [lldb] [lldb] Updated TCPSocket to listen multiple ports on the same single thread (PR #104797)

[Pavel Labath via lldb-commits]

labath wrote:

> @labath
> 
> > Having a single socket listen on multiple ports sounds like a bad idea to me.
> 
> Note that currently the class TCPSocket already contains a list of NativeSocket `m_listen_sockets`.

I am aware of that, and I'm not entirely happy with how the class implements listening. I think it would be better to have a separate class for a listening socket, because those need a completely different APIs. But, even ignoring that, I think there's a difference between listening on two addresses with the same port (I believe the only way to reach that state is if a hostname resolves to multiple addresses, in which case one really could argue that it's the same "logical" address), and two addresses with completely unrelated ports.

> We do not need 2 TCPSocket instances with 2 separated lists of native sockets even with a common MainLoop.
> 
> We have 2 options:
> 
>     * A) 2 threads - first one calls TCPSocket::Accept() for the platform connection, second calls TCPSocket::Accept() for the gdb connection
> 
>     * B) 1 thread - a common TCPSocket::Accept() can accept platform and gdb connections from both ports

We have (at least) three options, the third one being what I outlined in the previous message. I'm sorry this work of yours is going to waste. I could've been more explicit about what I wanted to do on the first PR, but I was more focused on what I don't want to do.

> 
> 
> I have implemented the option B. It was easy because the class TCPSocket already contains `m_listen_sockets`.
> 
> > Then we could create two sockets and have them listen on the same main loop instance.
> 
> It is already implemented inside TCPSocket but for different addresses. 

That's true, but I draw a different conclusion from that. Instead of saying "this should be extended to support multiple ports", my take is that "this wasn't a good place for multiplexing to begin with".

> I just added different ports.
> 
> The changes are minimal really. 

Yes, but that's because you extended a stringly typed api to do that you wanted. That function is used from other places as well, and this means that other places in lldb (including user-facing code, I believe) can accept these multi-port connection strings. I don't think we want to support that.

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