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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
