[lldb] Add timeout argument to Socket::Accept #117691
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Allows us to stop waiting for a connection if it doesn't come in a certain amount of time. Right now, I'm keeping the status quo (infitnite wait) in the "production" code, but using smaller (finite) values in tests. (A lot of these tests create "loopback" connections, where a really short wait is sufficient: on linux at least even a poll (0s wait) is sufficient if the other end has connect()ed already, but this doesn't seem to be the case on Windows, so I'm using a 1s wait in these cases).
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesAllows us to stop waiting for a connection if it doesn't come in a certain amount of time. Right now, I'm keeping the status quo (infitnite wait) in the "production" code, but using smaller (finite) values in tests. (A lot of these tests create "loopback" connections, where a really short wait is sufficient: on linux at least even a poll (0s wait) is sufficient if the other end has connect()ed already, but this doesn't seem to be the case on Windows, so I'm using a 1s wait in these cases). Full diff: https://github.com/llvm/llvm-project/pull/117691.diff 8 Files Affected:
diff --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index 14468c98ac5a3a..0e542b05a085c6 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -13,6 +13,7 @@
#include <string>
#include "lldb/Host/MainLoopBase.h"
+#include "lldb/Utility/Timeout.h"
#include "lldb/lldb-private.h"
#include "lldb/Host/SocketAddress.h"
@@ -108,7 +109,7 @@ class Socket : public IOObject {
// Accept a single connection and "return" it in the pointer argument. This
// function blocks until the connection arrives.
- virtual Status Accept(Socket *&socket);
+ virtual Status Accept(const Timeout<std::micro> &timeout, Socket *&socket);
// Initialize a Tcp Socket object in listening mode. listen and accept are
// implemented separately because the caller may wish to manipulate or query
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index d69eb608204033..63396f7b4abc9c 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -460,7 +460,8 @@ NativeSocket Socket::CreateSocket(const int domain, const int type,
return sock;
}
-Status Socket::Accept(Socket *&socket) {
+Status Socket::Accept(const Timeout<std::micro> &timeout, Socket *&socket) {
+ socket = nullptr;
MainLoop accept_loop;
llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> expected_handles =
Accept(accept_loop,
@@ -470,7 +471,15 @@ Status Socket::Accept(Socket *&socket) {
});
if (!expected_handles)
return Status::FromError(expected_handles.takeError());
- return accept_loop.Run();
+ if (timeout) {
+ accept_loop.AddCallback(
+ [](MainLoopBase &loop) { loop.RequestTermination(); }, *timeout);
+ }
+ if (Status status = accept_loop.Run(); status.Fail())
+ return status;
+ if (socket)
+ return Status();
+ return Status(std::make_error_code(std::errc::timed_out));
}
NativeSocket Socket::AcceptSocket(NativeSocket sockfd, struct sockaddr *addr,
diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 8a03e47ef3d900..903bfc50def3aa 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -543,7 +543,7 @@ lldb::ConnectionStatus ConnectionFileDescriptor::AcceptSocket(
if (!error.Fail()) {
post_listen_callback(*listening_socket);
- error = listening_socket->Accept(accepted_socket);
+ error = listening_socket->Accept(/*timeout=*/std::nullopt, accepted_socket);
}
if (!error.Fail()) {
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 7eacd605362e7c..67b41b1e77a533 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -1223,10 +1223,6 @@ GDBRemoteCommunication::ConnectLocally(GDBRemoteCommunication &client,
listen_socket.Listen("localhost:0", backlog).ToError())
return error;
- Socket *accept_socket = nullptr;
- std::future<Status> accept_status = std::async(
- std::launch::async, [&] { return listen_socket.Accept(accept_socket); });
-
llvm::SmallString<32> remote_addr;
llvm::raw_svector_ostream(remote_addr)
<< "connect://localhost:" << listen_socket.GetLocalPortNumber();
@@ -1238,10 +1234,15 @@ GDBRemoteCommunication::ConnectLocally(GDBRemoteCommunication &client,
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"Unable to connect: %s", status.AsCString());
- client.SetConnection(std::move(conn_up));
- if (llvm::Error error = accept_status.get().ToError())
- return error;
+ // The connection was already established above, so a short timeout is
+ // sufficient.
+ Socket *accept_socket = nullptr;
+ if (Status accept_status =
+ listen_socket.Accept(std::chrono::seconds(1), accept_socket);
+ accept_status.Fail())
+ return accept_status.takeError();
+ client.SetConnection(std::move(conn_up));
server.SetConnection(
std::make_unique<ConnectionFileDescriptor>(accept_socket));
return llvm::Error::success();
diff --git a/lldb/unittests/Host/MainLoopTest.cpp b/lldb/unittests/Host/MainLoopTest.cpp
index e7425b737a6dab..462aae50850438 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -42,7 +42,8 @@ class MainLoopTest : public testing::Test {
llvm::formatv("localhost:{0}", listen_socket_up->GetLocalPortNumber())
.str());
ASSERT_TRUE(error.Success());
- ASSERT_TRUE(listen_socket_up->Accept(accept_socket).Success());
+ ASSERT_TRUE(listen_socket_up->Accept(std::chrono::seconds(1), accept_socket)
+ .Success());
callback_count = 0;
socketpair[0] = std::move(connect_socket_up);
diff --git a/lldb/unittests/Host/SocketTest.cpp b/lldb/unittests/Host/SocketTest.cpp
index 4befddc0e21ce3..c020f1bff04798 100644
--- a/lldb/unittests/Host/SocketTest.cpp
+++ b/lldb/unittests/Host/SocketTest.cpp
@@ -12,7 +12,9 @@
#include "lldb/Host/MainLoop.h"
#include "lldb/Utility/UriParser.h"
#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <chrono>
using namespace lldb_private;
@@ -133,6 +135,27 @@ TEST_P(SocketTest, TCPListen0ConnectAccept) {
&socket_b_up);
}
+TEST_P(SocketTest, TCPAcceptTimeout) {
+ if (!HostSupportsProtocol())
+ return;
+
+ 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());
+
+ Socket *socket;
+ ASSERT_THAT_ERROR(
+ listen_socket_up->Accept(std::chrono::milliseconds(10), socket)
+ .takeError(),
+ llvm::Failed<llvm::ErrorInfoBase>(
+ testing::Property(&llvm::ErrorInfoBase::convertToErrorCode,
+ std::make_error_code(std::errc::timed_out))));
+}
+
TEST_P(SocketTest, TCPMainLoopAccept) {
if (!HostSupportsProtocol())
return;
diff --git a/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp b/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
index 2455a4f6f5d490..417900e7674dca 100644
--- a/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
+++ b/lldb/unittests/TestingSupport/Host/SocketTestUtilities.cpp
@@ -19,11 +19,6 @@
using namespace lldb_private;
-static void AcceptThread(Socket *listen_socket, bool child_processes_inherit,
- Socket **accept_socket, Status *error) {
- *error = listen_socket->Accept(*accept_socket);
-}
-
template <typename SocketType>
void lldb_private::CreateConnectedSockets(
llvm::StringRef listen_remote_address,
@@ -38,12 +33,6 @@ void lldb_private::CreateConnectedSockets(
ASSERT_THAT_ERROR(error.ToError(), llvm::Succeeded());
ASSERT_TRUE(listen_socket_up->IsValid());
- Status accept_error;
- Socket *accept_socket;
- std::thread accept_thread(AcceptThread, listen_socket_up.get(),
- child_processes_inherit, &accept_socket,
- &accept_error);
-
std::string connect_remote_address = get_connect_addr(*listen_socket_up);
std::unique_ptr<SocketType> connect_socket_up(
new SocketType(true, child_processes_inherit));
@@ -55,9 +44,13 @@ void lldb_private::CreateConnectedSockets(
a_up->swap(connect_socket_up);
ASSERT_TRUE((*a_up)->IsValid());
- accept_thread.join();
+ Socket *accept_socket;
+ ASSERT_THAT_ERROR(
+ listen_socket_up->Accept(std::chrono::seconds(1), accept_socket)
+ .takeError(),
+ llvm::Succeeded());
+
b_up->reset(static_cast<SocketType *>(accept_socket));
- ASSERT_THAT_ERROR(accept_error.ToError(), llvm::Succeeded());
ASSERT_NE(nullptr, b_up->get());
ASSERT_TRUE((*b_up)->IsValid());
diff --git a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
index a6f2dc32c6d0c0..9cbdf5278816d1 100644
--- a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -26,9 +26,13 @@ using namespace lldb_private;
using namespace llvm;
using namespace llgs_tests;
+static std::chrono::seconds GetDefaultTimeout() {
+ return std::chrono::seconds{10};
+}
+
TestClient::TestClient(std::unique_ptr<Connection> Conn) {
SetConnection(std::move(Conn));
- SetPacketTimeout(std::chrono::seconds(10));
+ SetPacketTimeout(GetDefaultTimeout());
}
TestClient::~TestClient() {
@@ -117,7 +121,10 @@ TestClient::launchCustom(StringRef Log, bool disable_stdio,
return status.ToError();
Socket *accept_socket;
- listen_socket.Accept(accept_socket);
+ if (llvm::Error E =
+ listen_socket.Accept(2 * GetDefaultTimeout(), accept_socket)
+ .takeError())
+ return E;
auto Conn = std::make_unique<ConnectionFileDescriptor>(accept_socket);
auto Client = std::unique_ptr<TestClient>(new TestClient(std::move(Conn)));
|
labath
commented
Nov 26, 2024
| @@ -543,7 +543,7 @@ lldb::ConnectionStatus ConnectionFileDescriptor::AcceptSocket( | |||
|
|
|||
| if (!error.Fail()) { | |||
| post_listen_callback(*listening_socket); | |||
| error = listening_socket->Accept(accepted_socket); | |||
| error = listening_socket->Accept(/*timeout=*/std::nullopt, accepted_socket); | |||
There was a problem hiding this comment.
@tedwoodward I think this is the timeout you want to reduce. I'm leaving it up to you to figure out how to do that (whether to use a fixed value, setting, whatever...).
|
I'm not complaining, but wanted to point out it feels a bit backwards to be adding a callback to timeout, rather than having |
|
I can sort of see your point, but I also disagree with it. :) It's true that in this case (the blocking version of Socket::Accept), a timeout argument to MainLoop::Run would be sufficient (and probably look cleaner), but it's not very generic. Like, imagine you had a "proper" long-lived MainLoop object (like the one we have in lldb-server), and you wanted to use that to wait for a connection. You couldn't pass a timeout to the Run function there. Instead, you'd probably schedule a callback to unregister the accept handle after a certain amount of time. In this mindset, I view the blocking Accept function as a sort of a convenience wrapper on top of that functionality and its goal is to provide a simple interface for the cases where all you want to do wait for a connection. |
|
FWIW, I think of MainLoop::Run as an equivalent to asyncio.loop.run_forever, which also doesn't take a timeout argument. The library does have a |
mgorny
approved these changes
Nov 26, 2024
|
Looks like this broke the macOS CI: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/16045/execution/node/97/log/ Mind fixing up that test? |
labath
added a commit
that referenced
this pull request
Nov 27, 2024
|
Fixed in 284d4e0. FWIW, I did not get any emails for that breakage. |
Thanks! Yea IIRC email notifications for green dragon are disabled at the moment (@JDevlieghere @adrian-prantl any idea why?) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allows us to stop waiting for a connection if it doesn't come in a certain amount of time. Right now, I'm keeping the status quo (infitnite wait) in the "production" code, but using smaller (finite) values in tests. (A lot of these tests create "loopback" connections, where a really short wait is sufficient: on linux at least even a poll (0s wait) is sufficient if the other end has connect()ed already, but this doesn't seem to be the case on Windows, so I'm using a 1s wait in these cases).