[Lldb-commits] [lldb] [lldb-dap] Use existing lldb::IOObjectSP for DAP IO (NFC). (PR #128750)

[Pavel Labath via lldb-commits]

https://github.com/labath approved this pull request.

This looks really nice. I also think the Input/OutputStreams could go away but that can be a separate patch. 

*However*, this part

>  properly detect shutdown requests when the Socket is closed

really scares me. I must have missed this during the review, but there is *no way* to reliably terminate thread by closing a file descriptor from under it. There are at least two bad scenarios here:

1. the socket FD gets reassigned before the thread manages to notice that, and it will start reading from a random socket. (This is more likely than it may seem because posix systems always assign the lowest available FD number)
2. On linux at least, if the reading thread is blocked (at least in a select syscall, but probably read as well) on that fd, then closing the fd will *not* unblock the thread.

To reliably force termination, I'd suggest changing the read thread to use a MainLoop object reading from the socket, and then terminating it via `loop.AddPendingCallback([&](MainLoopBase &loop) { loop.RequestTermination(); });`. It sounds a bit like overkill, but the main loop class is currently our multiplexing primitive with the widest platform support. It also goes along nicely with the IOObject classes you're introducing here. And when/if we have a way (see the Pipe patch) for it to handle pipes as well, we could have it handle stdout/err forwarding as well, and ditch the forwarding threads. 

(I can also imagine a setup where we reuse the top-level MainLoop object (the one that currently accepts connections). In this world the main thread would pull json data from (all) connections, and enqueue them to connection-specific request queues. The nice part about this approach is that there are fewer threads flying around, so it's easier to understand what's happening and easier to shut things down. The disadvantage is that the main thread becomes a bit of a choke point -- though I don't expect this to matter much in practice since we will usually only have one connection and I doubt that json (de)serialization is going to be the bottleneck)

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