[lldb] Add a callback version of TCPSocket::Accept #106955
Conversation
The existing function already used the MainLoop class, which allows one to wait on multiple events at once. It needed to do this in order to wait for v4 and v6 connections simultaneously. However, since it was creating its own instance of MainLoop, this meant that it was impossible to multiplex these sockets with anything else. This patch simply adds a version of this function which uses an externally provided main loop instance, which allows the caller to add any events it deems necessary. The previous function becomes a very thin wrapper over the new one.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThe existing function already used the MainLoop class, which allows one to wait on multiple events at once. It needed to do this in order to wait for v4 and v6 connections simultaneously. However, since it was creating its own instance of MainLoop, this meant that it was impossible to multiplex these sockets with anything else. This patch simply adds a version of this function which uses an externally provided main loop instance, which allows the caller to add any events it deems necessary. The previous function becomes a very thin wrapper over the new one. Full diff: https://github.com/llvm/llvm-project/pull/106955.diff 3 Files Affected:
diff --git a/lldb/include/lldb/Host/common/TCPSocket.h b/lldb/include/lldb/Host/common/TCPSocket.h
index b782c9e6096c44..78e80568e39967 100644
--- a/lldb/include/lldb/Host/common/TCPSocket.h
+++ b/lldb/include/lldb/Host/common/TCPSocket.h
@@ -9,6 +9,7 @@
#ifndef LLDB_HOST_COMMON_TCPSOCKET_H
#define LLDB_HOST_COMMON_TCPSOCKET_H
+#include "lldb/Host/MainLoopBase.h"
#include "lldb/Host/Socket.h"
#include "lldb/Host/SocketAddress.h"
#include <map>
@@ -40,6 +41,16 @@ class TCPSocket : public Socket {
Status Connect(llvm::StringRef name) override;
Status Listen(llvm::StringRef name, int backlog) override;
+
+ // Use the provided main loop instance to accept new connections. The callback
+ // will be called (from MainLoop::Run) for each new connection. This function
+ // does not block.
+ llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>>
+ Accept(MainLoopBase &loop,
+ std::function<void(std::unique_ptr<TCPSocket> socket)> sock_cb);
+
+ // Accept a single connection and "return" it in the pointer argument. This
+ // function blocks until the connection arrives.
Status Accept(Socket *&conn_socket) override;
Status CreateSocket(int domain);
diff --git a/lldb/source/Host/common/TCPSocket.cpp b/lldb/source/Host/common/TCPSocket.cpp
index ea26d8433c370a..4699e34bff924c 100644
--- a/lldb/source/Host/common/TCPSocket.cpp
+++ b/lldb/source/Host/common/TCPSocket.cpp
@@ -19,6 +19,7 @@
#include "llvm/Config/llvm-config.h"
#include "llvm/Support/Errno.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/WindowsError.h"
#include "llvm/Support/raw_ostream.h"
@@ -254,67 +255,60 @@ void TCPSocket::CloseListenSockets() {
m_listen_sockets.clear();
}
-Status TCPSocket::Accept(Socket *&conn_socket) {
- Status error;
- if (m_listen_sockets.size() == 0) {
- error = Status::FromErrorString("No open listening sockets!");
- return error;
- }
+llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> TCPSocket::Accept(
+ MainLoopBase &loop,
+ std::function<void(std::unique_ptr<TCPSocket> socket)> sock_cb) {
+ if (m_listen_sockets.size() == 0)
+ return llvm::createStringError("No open listening sockets!");
- NativeSocket sock = kInvalidSocketValue;
- NativeSocket listen_sock = kInvalidSocketValue;
- lldb_private::SocketAddress AcceptAddr;
- MainLoop accept_loop;
std::vector<MainLoopBase::ReadHandleUP> handles;
for (auto socket : m_listen_sockets) {
auto fd = socket.first;
- auto inherit = this->m_child_processes_inherit;
- auto io_sp = IOObjectSP(new TCPSocket(socket.first, false, inherit));
- handles.emplace_back(accept_loop.RegisterReadObject(
- io_sp, [fd, inherit, &sock, &AcceptAddr, &error,
- &listen_sock](MainLoopBase &loop) {
- socklen_t sa_len = AcceptAddr.GetMaxLength();
- sock = AcceptSocket(fd, &AcceptAddr.sockaddr(), &sa_len, inherit,
- error);
- listen_sock = fd;
- loop.RequestTermination();
- }, error));
- if (error.Fail())
- return error;
- }
+ auto io_sp =
+ std::make_shared<TCPSocket>(fd, false, this->m_child_processes_inherit);
+ auto cb = [this, fd, sock_cb](MainLoopBase &loop) {
+ lldb_private::SocketAddress AcceptAddr;
+ socklen_t sa_len = AcceptAddr.GetMaxLength();
+ Status error;
+ NativeSocket sock = AcceptSocket(fd, &AcceptAddr.sockaddr(), &sa_len,
+ m_child_processes_inherit, error);
+ Log *log = GetLog(LLDBLog::Host);
+ if (error.Fail())
+ LLDB_LOG(log, "AcceptSocket({0}): {1}", fd, error);
+
+ const lldb_private::SocketAddress &AddrIn = m_listen_sockets[fd];
+ if (!AddrIn.IsAnyAddr() && AcceptAddr != AddrIn) {
+ CLOSE_SOCKET(sock);
+ LLDB_LOG(log, "rejecting incoming connection from {0} (expecting {1})",
+ AcceptAddr.GetIPAddress(), AddrIn.GetIPAddress());
+ }
+ std::unique_ptr<TCPSocket> sock_up(new TCPSocket(sock, *this));
- bool accept_connection = false;
- std::unique_ptr<TCPSocket> accepted_socket;
- // Loop until we are happy with our connection
- while (!accept_connection) {
- accept_loop.Run();
+ // Keep our TCP packets coming without any delays.
+ sock_up->SetOptionNoDelay();
+ sock_cb(std::move(sock_up));
+ };
+ Status error;
+ handles.emplace_back(loop.RegisterReadObject(io_sp, cb, error));
if (error.Fail())
- return error;
-
- lldb_private::SocketAddress &AddrIn = m_listen_sockets[listen_sock];
- if (!AddrIn.IsAnyAddr() && AcceptAddr != AddrIn) {
- if (sock != kInvalidSocketValue) {
- CLOSE_SOCKET(sock);
- sock = kInvalidSocketValue;
- }
- llvm::errs() << llvm::formatv(
- "error: rejecting incoming connection from {0} (expecting {1})",
- AcceptAddr.GetIPAddress(), AddrIn.GetIPAddress());
- continue;
- }
- accept_connection = true;
- accepted_socket.reset(new TCPSocket(sock, *this));
+ return error.ToError();
}
- if (!accepted_socket)
- return error;
+ return handles;
+}
- // Keep our TCP packets coming without any delays.
- accepted_socket->SetOptionNoDelay();
- error.Clear();
- conn_socket = accepted_socket.release();
- return error;
+Status TCPSocket::Accept(Socket *&conn_socket) {
+ MainLoop accept_loop;
+ llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> expected_handles =
+ Accept(accept_loop,
+ [&accept_loop, &conn_socket](std::unique_ptr<TCPSocket> sock) {
+ conn_socket = sock.release();
+ accept_loop.RequestTermination();
+ });
+ if (!expected_handles)
+ return Status(expected_handles.takeError());
+ return accept_loop.Run();
}
int TCPSocket::SetOptionNoDelay() {
diff --git a/lldb/unittests/Host/SocketTest.cpp b/lldb/unittests/Host/SocketTest.cpp
index 78e1e11df06562..43650a4e3b8d2c 100644
--- a/lldb/unittests/Host/SocketTest.cpp
+++ b/lldb/unittests/Host/SocketTest.cpp
@@ -9,6 +9,7 @@
#include "TestingSupport/Host/SocketTestUtilities.h"
#include "TestingSupport/SubsystemRAII.h"
#include "lldb/Host/Config.h"
+#include "lldb/Host/MainLoop.h"
#include "lldb/Utility/UriParser.h"
#include "llvm/Testing/Support/Error.h"
#include "gtest/gtest.h"
@@ -95,6 +96,40 @@ TEST_P(SocketTest, TCPListen0ConnectAccept) {
&socket_b_up);
}
+TEST_P(SocketTest, TCPMainLoopAccept) {
+ const bool child_processes_inherit = false;
+ auto listen_socket_up =
+ std::make_unique<TCPSocket>(true, child_processes_inherit);
+ Status error = listen_socket_up->Listen(
+ llvm::formatv("[{0}]:0", GetParam().localhost_ip).str(), 5);
+ ASSERT_THAT_ERROR(error.ToError(), llvm::Succeeded());
+ ASSERT_TRUE(listen_socket_up->IsValid());
+
+ MainLoop loop;
+ std::unique_ptr<TCPSocket> accepted_socket_up;
+ auto expected_handles = listen_socket_up->Accept(
+ loop, [&accepted_socket_up, &loop](std::unique_ptr<TCPSocket> sock_up) {
+ accepted_socket_up = std::move(sock_up);
+ loop.RequestTermination();
+ });
+ ASSERT_THAT_EXPECTED(expected_handles, llvm::Succeeded());
+
+ std::unique_ptr<TCPSocket> connect_socket_up(
+ new TCPSocket(true, child_processes_inherit));
+ ASSERT_THAT_ERROR(
+ connect_socket_up
+ ->Connect(llvm::formatv("[{0}]:{1}", GetParam().localhost_ip,
+ listen_socket_up->GetLocalPortNumber())
+ .str())
+ .ToError(),
+ llvm::Succeeded());
+ ASSERT_TRUE(connect_socket_up->IsValid());
+
+ loop.Run();
+ ASSERT_TRUE(accepted_socket_up);
+ ASSERT_TRUE(accepted_socket_up->IsValid());
+}
+
TEST_P(SocketTest, TCPGetAddress) {
std::unique_ptr<TCPSocket> socket_a_up;
std::unique_ptr<TCPSocket> socket_b_up;
|
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. sock_cb parameter is useles here because it will be always the same. If you want to delete Acceptor, the best approach is the following: Use Listen() from #104797 but w/o changing the connection string: The accept MainLoop must be a member of TCPSocket and it is enough to just add
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 BTW, please approve NFC #106640. |
Note Acceptor is used ONLY in lldb-platform.cpp |
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.)
In a single thread -- yes. In the same blocking accept -- not really, at least not for me.
I guess we'll have to agree to disagree on that.
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".
Yes, and there's no reason to change that. (I'd definitely like to change that, but that's not necessary here)
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:
If it turns out to be easier, I'd be fine with turning |
| auto listen_socket_up = | ||
| std::make_unique<TCPSocket>(true, child_processes_inherit); | ||
| Status error = listen_socket_up->Listen( | ||
| llvm::formatv("[{0}]:0", GetParam().localhost_ip).str(), 5); |
There was a problem hiding this comment.
Probably it is necessary to add the following to avoid failure with IPv6
if (!HostSupportsProtocol())
return;
There was a problem hiding this comment.
BTW, I have figured out a side effect. MainLoopWindows uses events created by WSACreateEvent(). WSACreateEvent() creates the manual-reset event. But WSAResetEvent() is used only for m_trigger_event. All other events (READ, ACCEPT, CLOSE) are never reset. I have no idea how it worked before. But I got thousands of error messages using this patch with lldb-server after first connection:
AcceptSocket(): A non-blocking socket operation could not be completed immediately
I have added WSAResetEvent(KV.second.event); before ProcessReadObject(KV.first); in MainLoopWindows.cpp to fix it. It seems this issue must be fixed before or with this patch, otherwise it will break the Windows build.
| if (error.Fail()) | ||
| LLDB_LOG(log, "AcceptSocket({0}): {1}", fd, error); | ||
|
|
||
| const lldb_private::SocketAddress &AddrIn = m_listen_sockets[fd]; | ||
| if (!AddrIn.IsAnyAddr() && AcceptAddr != AddrIn) { | ||
| CLOSE_SOCKET(sock); | ||
| LLDB_LOG(log, "rejecting incoming connection from {0} (expecting {1})", | ||
| AcceptAddr.GetIPAddress(), AddrIn.GetIPAddress()); | ||
| } | ||
| std::unique_ptr<TCPSocket> sock_up(new TCPSocket(sock, *this)); | ||
|
|
||
| bool accept_connection = false; | ||
| std::unique_ptr<TCPSocket> accepted_socket; | ||
| // Loop until we are happy with our connection | ||
| while (!accept_connection) { | ||
| accept_loop.Run(); | ||
| // Keep our TCP packets coming without any delays. | ||
| sock_up->SetOptionNoDelay(); | ||
|
|
||
| sock_cb(std::move(sock_up)); |
There was a problem hiding this comment.
Avoid call CLOSE_SOCKET(sock) or create the TCPSocket instance with an invalid sock.
if (error.Fail())
LLDB_LOG(log, "AcceptSocket({0}): {1}", fd, error);
else {
const lldb_private::SocketAddress &AddrIn = m_listen_sockets[fd];
if (!AddrIn.IsAnyAddr() && AcceptAddr != AddrIn) {
CLOSE_SOCKET(sock);
LLDB_LOG(log,
"rejecting incoming connection from {0} (expecting {1})",
AcceptAddr.GetIPAddress(), AddrIn.GetIPAddress());
} else {
std::unique_ptr<TCPSocket> sock_up(new TCPSocket(sock, *this));
// Keep our TCP packets coming without any delays.
sock_up->SetOptionNoDelay();
sock_cb(std::move(sock_up));
}
}
There was a problem hiding this comment.
Done (using early returns). Thanks for catching that.
Listen to gdbserver-port, accept the connection and run lldb-server gdbserver --fd on all platforms. Parameters --min-gdbserver-port and --max-gdbserver-port are deprecated now. This is the part 2 of llvm#101283. Depends on llvm#106955. Fixes llvm#97537, fixes llvm#101475.
This prevents the callback function from being called in a busy loop. Discovered by @slydiman on llvm#106955.
Oops. Thanks for catching that. I think have a pretty good idea of how this worked before. For accepts, we were always creating a fresh MainLoop instance (and a WSAEvent to go along with it), so this wasn't an issue. And for reads, we probably were just spinning in a busy loop after the first read event. I've created #107061 to fix that. |
The existing function already used the MainLoop class, which allows one to wait on multiple events at once. It needed to do this in order to wait for v4 and v6 connections simultaneously. However, since it was creating its own instance of MainLoop, this meant that it was impossible to multiplex these sockets with anything else. This patch simply adds a version of this function which uses an externally provided main loop instance, which allows the caller to add any events it deems necessary. The previous function becomes a very thin wrapper over the new one. (cherry picked from commit 3d5e1ec)
The existing function already used the MainLoop class, which allows one to wait on multiple events at once. It needed to do this in order to wait for v4 and v6 connections simultaneously. However, since it was creating its own instance of MainLoop, this meant that it was impossible to multiplex these sockets with anything else.
This patch simply adds a version of this function which uses an externally provided main loop instance, which allows the caller to add any events it deems necessary. The previous function becomes a very thin wrapper over the new one.