[lldb] Adding file and pipe support to lldb_private::MainLoopWindows. #145621
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
ashgti
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ashgti
force-pushed
the
lldb-mainloopwindows
branch
4 times, most recently
from
0427c24 to
3db6bad
Compare
This updates MainLoopWindows to support events for reading from a file and a socket type. This unifies both handle types using WaitForMultipleEvents which can listen to both sockets and files for change events. This should allow us to unify how we handle watching files/pipes/sockets on Windows and Posix systems.
ashgti
force-pushed
the
lldb-mainloopwindows
branch
from
3db6bad to
954f107
Compare
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis updates MainLoopWindows to support events for reading from a file and a socket type. This unifies both handle types using This should allow us to unify how we handle watching files/pipes/sockets on Windows and Posix systems. Full diff: https://github.com/llvm/llvm-project/pull/145621.diff 11 Files Affected:
diff --git a/lldb/include/lldb/Host/File.h b/lldb/include/lldb/Host/File.h
index 9e2d0abe0b1af..36cb192281289 100644
--- a/lldb/include/lldb/Host/File.h
+++ b/lldb/include/lldb/Host/File.h
@@ -127,6 +127,7 @@ class File : public IOObject {
/// \return
/// a valid handle or IOObject::kInvalidHandleValue
WaitableHandle GetWaitableHandle() override;
+ bool HasReadableData() override;
/// Get the file specification for this file, if possible.
///
@@ -400,6 +401,7 @@ class NativeFile : public File {
Status Write(const void *buf, size_t &num_bytes) override;
Status Close() override;
WaitableHandle GetWaitableHandle() override;
+ bool HasReadableData() override;
Status GetFileSpec(FileSpec &file_spec) const override;
int GetDescriptor() const override;
FILE *GetStream() override;
diff --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index 89953ee7fd5b6..6569e9e6ea818 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -158,6 +158,7 @@ class Socket : public IOObject {
bool IsValid() const override { return m_socket != kInvalidSocketValue; }
WaitableHandle GetWaitableHandle() override;
+ bool HasReadableData() override;
static llvm::Expected<HostAndPort>
DecodeHostAndPort(llvm::StringRef host_and_port);
@@ -185,6 +186,7 @@ class Socket : public IOObject {
SocketProtocol m_protocol;
NativeSocket m_socket;
+ WaitableHandle m_waitable_handle;
bool m_should_close_fd;
};
diff --git a/lldb/include/lldb/Host/windows/MainLoopWindows.h b/lldb/include/lldb/Host/windows/MainLoopWindows.h
index 3937a24645d95..43b7d13a0e445 100644
--- a/lldb/include/lldb/Host/windows/MainLoopWindows.h
+++ b/lldb/include/lldb/Host/windows/MainLoopWindows.h
@@ -37,11 +37,10 @@ class MainLoopWindows : public MainLoopBase {
void Interrupt() override;
private:
- void ProcessReadObject(IOObject::WaitableHandle handle);
llvm::Expected<size_t> Poll();
struct FdInfo {
- void *event;
+ lldb::IOObjectSP object_sp;
Callback callback;
};
llvm::DenseMap<IOObject::WaitableHandle, FdInfo> m_read_fds;
diff --git a/lldb/include/lldb/Utility/IOObject.h b/lldb/include/lldb/Utility/IOObject.h
index 8cf42992e7be5..48a8a2076581f 100644
--- a/lldb/include/lldb/Utility/IOObject.h
+++ b/lldb/include/lldb/Utility/IOObject.h
@@ -14,6 +14,7 @@
#include <sys/types.h>
#include "lldb/lldb-private.h"
+#include "lldb/lldb-types.h"
namespace lldb_private {
@@ -24,9 +25,9 @@ class IOObject {
eFDTypeSocket, // Socket requiring send/recv
};
- // TODO: On Windows this should be a HANDLE, and wait should use
- // WaitForMultipleObjects
- typedef int WaitableHandle;
+ // A handle for integrating with the host event loop model.
+ using WaitableHandle = lldb::file_t;
+
static const WaitableHandle kInvalidHandleValue;
IOObject(FDType type) : m_fd_type(type) {}
@@ -40,6 +41,7 @@ class IOObject {
FDType GetFdType() const { return m_fd_type; }
virtual WaitableHandle GetWaitableHandle() = 0;
+ virtual bool HasReadableData() = 0;
protected:
FDType m_fd_type;
diff --git a/lldb/source/Host/common/File.cpp b/lldb/source/Host/common/File.cpp
index 9aa95ffda44cb..2d33f9e2028c4 100644
--- a/lldb/source/Host/common/File.cpp
+++ b/lldb/source/Host/common/File.cpp
@@ -118,6 +118,8 @@ IOObject::WaitableHandle File::GetWaitableHandle() {
return IOObject::kInvalidHandleValue;
}
+bool File::HasReadableData() { return false; }
+
Status File::GetFileSpec(FileSpec &file_spec) const {
file_spec.Clear();
return std::error_code(ENOTSUP, std::system_category());
@@ -274,7 +276,23 @@ int NativeFile::GetDescriptor() const {
}
IOObject::WaitableHandle NativeFile::GetWaitableHandle() {
+#ifdef _WIN32
+ return (HANDLE)_get_osfhandle(GetDescriptor());
+#else
return GetDescriptor();
+#endif
+}
+
+bool NativeFile::HasReadableData() {
+#ifdef _WIN32
+ DWORD available_bytes = 0;
+ return !PeekNamedPipe((HANDLE)_get_osfhandle(GetDescriptor()), NULL, 0, NULL,
+ &available_bytes, NULL) ||
+ available_bytes > 0;
+#else
+ size_t buffer_size = 0;
+ return ioctl(GetDescriptor(), FIONREAD, buffer_size) != -1 && buffer_size > 0;
+#endif
}
FILE *NativeFile::GetStream() {
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index 2b23fd1e6e57e..47ccbd94ce510 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -31,8 +31,11 @@
#include <netdb.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
+#include <sys/ioctl.h>
#include <sys/socket.h>
+#include <sys/stat.h>
#include <sys/un.h>
+#include <termios.h>
#include <unistd.h>
#endif
@@ -169,7 +172,9 @@ bool Socket::FindProtocolByScheme(const char *scheme,
Socket::Socket(SocketProtocol protocol, bool should_close)
: IOObject(eFDTypeSocket), m_protocol(protocol),
- m_socket(kInvalidSocketValue), m_should_close_fd(should_close) {}
+ m_socket(kInvalidSocketValue),
+ m_waitable_handle(IOObject::kInvalidHandleValue),
+ m_should_close_fd(should_close) {}
Socket::~Socket() { Close(); }
@@ -313,8 +318,39 @@ Socket::DecodeHostAndPort(llvm::StringRef host_and_port) {
}
IOObject::WaitableHandle Socket::GetWaitableHandle() {
- // TODO: On Windows, use WSAEventSelect
+#ifdef _WIN32
+ if (m_socket == kInvalidSocketValue)
+ return kInvalidHandleValue;
+
+ if (m_waitable_handle == kInvalidHandleValue) {
+ m_waitable_handle = WSACreateEvent();
+ assert(m_waitable_handle != WSA_INVALID_EVENT);
+ if (WSAEventSelect(m_socket, m_waitable_handle,
+ FD_ACCEPT | FD_READ | FD_WRITE) != 0)
+ return kInvalidHandleValue;
+ }
+
+ return m_waitable_handle;
+#else
return m_socket;
+#endif
+}
+
+bool Socket::HasReadableData() {
+#ifdef _WIN32
+ if (!IsValid() || m_waitable_handle == kInvalidHandleValue)
+ return false;
+
+ WSANETWORKEVENTS events;
+ if (WSAEnumNetworkEvents(m_socket, m_waitable_handle, &events) != 0)
+ return false;
+
+ return events.lNetworkEvents & FD_CLOSE ||
+ events.lNetworkEvents & FD_ACCEPT || events.lNetworkEvents & FD_READ;
+#else
+ size_t buffer_size = 0;
+ return ioctl(m_socket, FIONREAD, buffer_size) != -1 && buffer_size > 0;
+#endif
}
Status Socket::Read(void *buf, size_t &num_bytes) {
@@ -380,7 +416,14 @@ Status Socket::Close() {
Log *log = GetLog(LLDBLog::Connection);
LLDB_LOGF(log, "%p Socket::Close (fd = %" PRIu64 ")",
static_cast<void *>(this), static_cast<uint64_t>(m_socket));
-
+#ifdef _WIN32
+ if (m_waitable_handle != kInvalidHandleValue) {
+ if (WSACloseEvent(m_waitable_handle) == 0)
+ m_waitable_handle = kInvalidHandleValue;
+ else
+ error = GetLastError();
+ }
+#endif
bool success = CloseSocket(m_socket) == 0;
// A reference to a FD was passed in, set it to an invalid value
m_socket = kInvalidSocketValue;
diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 57dce44812c89..2fcad7f193e1a 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -276,7 +276,7 @@ size_t ConnectionFileDescriptor::Read(void *dst, size_t dst_len,
"%p ConnectionFileDescriptor::Read() fd = %" PRIu64
", dst = %p, dst_len = %" PRIu64 ") => %" PRIu64 ", error = %s",
static_cast<void *>(this),
- static_cast<uint64_t>(m_io_sp->GetWaitableHandle()),
+ static_cast<file_t>(m_io_sp->GetWaitableHandle()),
static_cast<void *>(dst), static_cast<uint64_t>(dst_len),
static_cast<uint64_t>(bytes_read), error.AsCString());
}
@@ -380,7 +380,7 @@ size_t ConnectionFileDescriptor::Write(const void *src, size_t src_len,
"%p ConnectionFileDescriptor::Write(fd = %" PRIu64
", src = %p, src_len = %" PRIu64 ") => %" PRIu64 " (error = %s)",
static_cast<void *>(this),
- static_cast<uint64_t>(m_io_sp->GetWaitableHandle()),
+ static_cast<file_t>(m_io_sp->GetWaitableHandle()),
static_cast<const void *>(src), static_cast<uint64_t>(src_len),
static_cast<uint64_t>(bytes_sent), error.AsCString());
}
@@ -451,14 +451,17 @@ ConnectionFileDescriptor::BytesAvailable(const Timeout<std::micro> &timeout,
if (timeout)
select_helper.SetTimeout(*timeout);
- select_helper.FDSetRead(handle);
+ // FIXME: Migrate to MainLoop.
#if defined(_WIN32)
+ if (const auto *sock = static_cast<Socket *>(m_io_sp.get()))
+ select_helper.FDSetRead((socket_t)sock->GetNativeSocket());
// select() won't accept pipes on Windows. The entire Windows codepath
// needs to be converted over to using WaitForMultipleObjects and event
// HANDLEs, but for now at least this will allow ::select() to not return
// an error.
const bool have_pipe_fd = false;
#else
+ select_helper.FDSetRead(handle);
const bool have_pipe_fd = pipe_fd >= 0;
#endif
if (have_pipe_fd)
@@ -493,7 +496,12 @@ ConnectionFileDescriptor::BytesAvailable(const Timeout<std::micro> &timeout,
break; // Lets keep reading to until we timeout
}
} else {
+#if defined(_WIN32)
+ if (const auto *sock = static_cast<Socket *>(m_io_sp.get());
+ select_helper.FDIsSetRead(sock->GetNativeSocket()))
+#else
if (select_helper.FDIsSetRead(handle))
+#endif
return eConnectionStatusSuccess;
if (select_helper.FDIsSetRead(pipe_fd)) {
diff --git a/lldb/source/Host/windows/MainLoopWindows.cpp b/lldb/source/Host/windows/MainLoopWindows.cpp
index f3ab2a710cd01..33cc06266ce03 100644
--- a/lldb/source/Host/windows/MainLoopWindows.cpp
+++ b/lldb/source/Host/windows/MainLoopWindows.cpp
@@ -8,8 +8,11 @@
#include "lldb/Host/windows/MainLoopWindows.h"
#include "lldb/Host/Config.h"
+#include "lldb/Host/Socket.h"
#include "lldb/Utility/Status.h"
#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/WindowsError.h"
#include <algorithm>
#include <cassert>
#include <cerrno>
@@ -21,16 +24,6 @@
using namespace lldb;
using namespace lldb_private;
-static DWORD ToTimeout(std::optional<MainLoopWindows::TimePoint> point) {
- using namespace std::chrono;
-
- if (!point)
- return WSA_INFINITE;
-
- nanoseconds dur = (std::max)(*point - steady_clock::now(), nanoseconds(0));
- return ceil<milliseconds>(dur).count();
-}
-
MainLoopWindows::MainLoopWindows() {
m_interrupt_event = WSACreateEvent();
assert(m_interrupt_event != WSA_INVALID_EVENT);
@@ -44,36 +37,61 @@ MainLoopWindows::~MainLoopWindows() {
}
llvm::Expected<size_t> MainLoopWindows::Poll() {
- std::vector<WSAEVENT> events;
+ std::vector<HANDLE> events;
events.reserve(m_read_fds.size() + 1);
- for (auto &[fd, info] : m_read_fds) {
- int result = WSAEventSelect(fd, info.event, FD_READ | FD_ACCEPT | FD_CLOSE);
- assert(result == 0);
- UNUSED_IF_ASSERT_DISABLED(result);
-
- events.push_back(info.event);
+ for (auto &[fd, fd_info] : m_read_fds) {
+ // short circuit waiting if a handle is already ready.
+ if (fd_info.object_sp->HasReadableData())
+ return events.size();
+ events.push_back(fd);
}
events.push_back(m_interrupt_event);
- DWORD result =
- WSAWaitForMultipleEvents(events.size(), events.data(), FALSE,
- ToTimeout(GetNextWakeupTime()), FALSE);
+ while (true) {
+ DWORD timeout = INFINITY;
+ std::optional<lldb_private::MainLoopBase::TimePoint> deadline =
+ GetNextWakeupTime();
+ if (deadline) {
+ // Check how much time is remaining, we may have woken up early for an
+ // unrelated reason on a file descriptor (e.g. a stat was triggered).
+ std::chrono::milliseconds remaining =
+ std::chrono::duration_cast<std::chrono::milliseconds>(
+ deadline.value() - std::chrono::steady_clock::now());
+ if (remaining.count() <= 0)
+ return events.size() - 1;
+ timeout = remaining.count();
+ }
- for (auto &fd : m_read_fds) {
- int result = WSAEventSelect(fd.first, WSA_INVALID_EVENT, 0);
- assert(result == 0);
- UNUSED_IF_ASSERT_DISABLED(result);
+ DWORD result =
+ WaitForMultipleObjects(events.size(), events.data(), FALSE, timeout);
+
+ // A timeout is treated as a (premature) signalization of the interrupt
+ // event.
+ if (result == WAIT_TIMEOUT)
+ return events.size() - 1;
+
+ if (result == WAIT_FAILED)
+ return llvm::createStringError(llvm::mapLastWindowsError(),
+ "WaitForMultipleObjects failed");
+
+ // check if interrupt requested.
+ if (result == WAIT_OBJECT_0 + events.size())
+ return result - WAIT_OBJECT_0;
+
+ // An object may be signaled before data is ready for reading, verify it has
+ // data.
+ if (result >= WAIT_OBJECT_0 &&
+ result < WAIT_OBJECT_0 + (events.size() - 1) &&
+ std::next(m_read_fds.begin(), result - WAIT_OBJECT_0)
+ ->second.object_sp->HasReadableData())
+ return result - WAIT_OBJECT_0;
+
+ // If no handles are actually ready then yield the thread to allow the CPU
+ // to progress.
+ std::this_thread::yield();
}
- if (result >= WSA_WAIT_EVENT_0 && result < WSA_WAIT_EVENT_0 + events.size())
- return result - WSA_WAIT_EVENT_0;
-
- // A timeout is treated as a (premature) signalization of the interrupt event.
- if (result == WSA_WAIT_TIMEOUT)
- return events.size() - 1;
-
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "WSAWaitForMultipleEvents failed");
+ llvm_unreachable();
}
MainLoopWindows::ReadHandleUP
@@ -83,28 +101,16 @@ MainLoopWindows::RegisterReadObject(const IOObjectSP &object_sp,
error = Status::FromErrorString("IO object is not valid.");
return nullptr;
}
- if (object_sp->GetFdType() != IOObject::eFDTypeSocket) {
- error = Status::FromErrorString(
- "MainLoopWindows: non-socket types unsupported on Windows");
- return nullptr;
- }
- WSAEVENT event = WSACreateEvent();
- if (event == WSA_INVALID_EVENT) {
- error =
- Status::FromErrorStringWithFormat("Cannot create monitoring event.");
- return nullptr;
- }
+ IOObject::WaitableHandle waitable_handle = object_sp->GetWaitableHandle();
+ assert(waitable_handle != IOObject::kInvalidHandleValue);
const bool inserted =
- m_read_fds
- .try_emplace(object_sp->GetWaitableHandle(), FdInfo{event, callback})
+ m_read_fds.try_emplace(waitable_handle, FdInfo{object_sp, callback})
.second;
if (!inserted) {
- WSACloseEvent(event);
error = Status::FromErrorStringWithFormat(
- "File descriptor %d already monitored.",
- object_sp->GetWaitableHandle());
+ "File descriptor %d already monitored.", waitable_handle);
return nullptr;
}
@@ -114,18 +120,9 @@ MainLoopWindows::RegisterReadObject(const IOObjectSP &object_sp,
void MainLoopWindows::UnregisterReadObject(IOObject::WaitableHandle handle) {
auto it = m_read_fds.find(handle);
assert(it != m_read_fds.end());
- BOOL result = WSACloseEvent(it->second.event);
- assert(result == TRUE);
- UNUSED_IF_ASSERT_DISABLED(result);
m_read_fds.erase(it);
}
-void MainLoopWindows::ProcessReadObject(IOObject::WaitableHandle handle) {
- auto it = m_read_fds.find(handle);
- if (it != m_read_fds.end())
- it->second.callback(*this); // Do the work
-}
-
Status MainLoopWindows::Run() {
m_terminate_request = false;
@@ -138,8 +135,7 @@ Status MainLoopWindows::Run() {
if (*signaled_event < m_read_fds.size()) {
auto &KV = *std::next(m_read_fds.begin(), *signaled_event);
- WSAResetEvent(KV.second.event);
- ProcessReadObject(KV.first);
+ KV.second.callback(*this); // Do the work.
} else {
assert(*signaled_event == m_read_fds.size());
WSAResetEvent(m_interrupt_event);
diff --git a/lldb/source/Utility/IOObject.cpp b/lldb/source/Utility/IOObject.cpp
index 964edce0ce10d..c0c07cc0b68e3 100644
--- a/lldb/source/Utility/IOObject.cpp
+++ b/lldb/source/Utility/IOObject.cpp
@@ -8,7 +8,16 @@
#include "lldb/Utility/IOObject.h"
+#ifdef _WIN32
+#include "lldb/Host/windows/windows.h"
+#endif
+
using namespace lldb_private;
+#ifdef _WIN32
+const IOObject::WaitableHandle IOObject::kInvalidHandleValue =
+ INVALID_HANDLE_VALUE;
+#else
const IOObject::WaitableHandle IOObject::kInvalidHandleValue = -1;
+#endif
IOObject::~IOObject() = default;
diff --git a/lldb/unittests/Host/FileTest.cpp b/lldb/unittests/Host/FileTest.cpp
index 35c87bb200fad..d973d19430596 100644
--- a/lldb/unittests/Host/FileTest.cpp
+++ b/lldb/unittests/Host/FileTest.cpp
@@ -14,6 +14,10 @@
#include "llvm/Support/Program.h"
#include "gtest/gtest.h"
+#ifdef _WIN32
+#include "lldb/Host/windows/windows.h"
+#endif
+
using namespace lldb;
using namespace lldb_private;
@@ -32,7 +36,11 @@ TEST(File, GetWaitableHandleFileno) {
ASSERT_TRUE(stream);
NativeFile file(stream, true);
- EXPECT_EQ(file.GetWaitableHandle(), fd);
+#ifdef _WIN32
+ EXPECT_EQ(file.GetWaitableHandle(), (HANDLE)_get_osfhandle(fd));
+#else
+ EXPECT_EQ(file.GetWaitableHandle(), (file_t)fd);
+#endif
}
TEST(File, GetStreamFromDescriptor) {
@@ -53,5 +61,9 @@ TEST(File, GetStreamFromDescriptor) {
ASSERT_TRUE(stream != NULL);
EXPECT_EQ(file.GetDescriptor(), fd);
- EXPECT_EQ(file.GetWaitableHandle(), fd);
+#ifdef _WIN32
+ EXPECT_EQ(file.GetWaitableHandle(), (HANDLE)_get_osfhandle(fd));
+#else
+ EXPECT_EQ(file.GetWaitableHandle(), (file_t)fd);
+#endif
}
diff --git a/lldb/unittests/Host/MainLoopTest.cpp b/lldb/unittests/Host/MainLoopTest.cpp
index e18489978e90c..b8d7f57108017 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -79,6 +79,28 @@ TEST_F(MainLoopTest, ReadObject) {
ASSERT_EQ(1u, callback_count);
}
+TEST_F(MainLoopTest, ReadPipeObject) {
+ Pipe pipe;
+
+ ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded());
+
+ MainLoop loop;
+
+ char X = 'X';
+ size_t len = sizeof(X);
+ ASSERT_THAT_EXPECTED(pipe.Write(&X, len), llvm::Succeeded());
+
+ Status error;
+ auto handle = loop.RegisterReadObject(
+ std::make_shared<NativeFile>(pipe.GetReadFileDescriptor(),
+ File::eOpenOptionReadOnly, false),
+ make_callback(), error);
+ ASSERT_TRUE(error.Success());
+ ASSERT_TRUE(handle);
+ ASSERT_TRUE(loop.Run().Success());
+ ASSERT_EQ(1u, callback_count);
+}
+
TEST_F(MainLoopTest, NoSpuriousReads) {
// Write one byte into the socket.
char X = 'X';
@@ -166,6 +188,10 @@ TEST_F(MainLoopTest, PendingCallbackCalledOnlyOnce) {
if (callback_count == 0) {
loop.AddPendingCallback([&](MainLoopBase &loop) {
callback_count++;
+ char X = 'X';
+ size_t len = sizeof(X);
+ // Write to trigger read object again.
+ ASSERT_TRUE(socketpair[0]->Write(&X, len).Success());
});
}
// Terminate the loop on second iteration.
|
labath
reviewed
Jun 26, 2025
There was a problem hiding this comment.
Thank you again for looking into this.
I think that the most important question we need to figure out is what's the danger of this turning into a busy wait. A function like HasReadableData could be useful in some situation, so I don't have a problem with it per-se, but the way you're using it makes me very suspicious that can turn into a busy wait under some circumstances. I think we at least need to know what those circumstances are, so we can evaluate whether that worth the (very tangible) benefit of being able to use this functionality portably.
| events.lNetworkEvents & FD_ACCEPT || events.lNetworkEvents & FD_READ; | ||
| #else | ||
| size_t buffer_size = 0; | ||
| return ioctl(m_socket, FIONREAD, buffer_size) != -1 && buffer_size > 0; |
There was a problem hiding this comment.
FIONREAD seems to be a thing on windows as well. Might be nice to use that, for consistency.
Or, are you relying on this returning true for "accepts" as well? If so, then I'm not sure I like this API.
Comment on lines
+322
to
+333
| if (m_socket == kInvalidSocketValue) | ||
| return kInvalidHandleValue; | ||
|
|
||
| if (m_waitable_handle == kInvalidHandleValue) { | ||
| m_waitable_handle = WSACreateEvent(); | ||
| assert(m_waitable_handle != WSA_INVALID_EVENT); | ||
| if (WSAEventSelect(m_socket, m_waitable_handle, | ||
| FD_ACCEPT | FD_READ | FD_WRITE) != 0) | ||
| return kInvalidHandleValue; | ||
| } | ||
|
|
||
| return m_waitable_handle; |
There was a problem hiding this comment.
I'd probably create the in the constructor rather than lazily. Besides clearer lifetimes, one might reasonably expect functions like GetWaitableHandle and HasReadableData to be thread safe.
Comment on lines
+51
to
+63
| DWORD timeout = INFINITY; | ||
| std::optional<lldb_private::MainLoopBase::TimePoint> deadline = | ||
| GetNextWakeupTime(); | ||
| if (deadline) { | ||
| // Check how much time is remaining, we may have woken up early for an | ||
| // unrelated reason on a file descriptor (e.g. a stat was triggered). | ||
| std::chrono::milliseconds remaining = | ||
| std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| deadline.value() - std::chrono::steady_clock::now()); | ||
| if (remaining.count() <= 0) | ||
| return events.size() - 1; | ||
| timeout = remaining.count(); | ||
| } |
There was a problem hiding this comment.
Is this any different from
DWARF timeout = ToTimeout(GetNextWakeupTime());
if (timeout == 0)
return events.size() - 1;
(using the original definition of the ToTimeout function) ?
Comment on lines
+89
to
+91
| // If no handles are actually ready then yield the thread to allow the CPU | ||
| // to progress. | ||
| std::this_thread::yield(); |
There was a problem hiding this comment.
What exactly are the situations when we reach this? I don't mind spinning here once or twice, but I'd hate to see this turn into busy wait...
| bool NativeFile::HasReadableData() { | ||
| #ifdef _WIN32 | ||
| DWORD available_bytes = 0; | ||
| return !PeekNamedPipe((HANDLE)_get_osfhandle(GetDescriptor()), NULL, 0, NULL, |
There was a problem hiding this comment.
This will only work for "files" that are actually pipes, right?
If so, I don't think that's a good long-term direction. My medium-to-long term plan would be to make Pipe an IOObject, but that requires splitting it into two (one IOObject for each end). I don't think that would be a particularly hard refactor, but if you don't want to take that on, maybe we can find a way to pass a Pipe to MainLoop directly (and keep NativeFile out of it)? I'd be fine with an RegisterReadObject overload that takes a Pipe (and ignores the write end of the pipe -- we don't select on writes anyway).
Comment on lines
+191
to
+194
| char X = 'X'; | ||
| size_t len = sizeof(X); | ||
| // Write to trigger read object again. | ||
| ASSERT_TRUE(socketpair[0]->Write(&X, len).Success()); |
There was a problem hiding this comment.
I think that making sure that this triggers without needing to exhaust or write to the pipe/socket is an important property to preserve.
Given that the POSIX waiting APIs are level-triggered, and that our codebase (still) has a very posix-y feel, I don't have much faith that all the callers (present and future) will correctly handle edge-triggered callbacks. I remember originally working around this with the WSAEventSelect(fd.first, WSA_INVALID_EVENT, 0). You might be able to do something similar here.
If that is hard, I suppose we could move to edge-triggered (or rather, indeterminate-triggered) callbacks, but we ought to do that as a separate patch.
| m_waitable_handle = WSACreateEvent(); | ||
| assert(m_waitable_handle != WSA_INVALID_EVENT); | ||
| if (WSAEventSelect(m_socket, m_waitable_handle, | ||
| FD_ACCEPT | FD_READ | FD_WRITE) != 0) |
There was a problem hiding this comment.
Does this mean that the event will be triggered whenever the socket is writable (triggering the busy wait I mentioned before)? That would definitely be undesirable.
If WSAEventSelect supports it, we might be able to deal with that by using separate events for reads and writes. The question is somewhat academic because noone is currently selecting for writes, but it's interesting from the perspective of where should that event live. You're moving it from the MainLoop to the Socket, which kind of makes sense, but it may not be possible if there isn't a single event (or a set of events) that can be used for everything.
In that world, I think that the original setup (where the event is managed inside the main loop) makes more sense as the loop can WSAEventSelect precisely the events it is interested it. And it should mostly (as long as they don't attempt that in parallel) work with sockets that are added to multiple main loops (which definitely works on posix).
In principle, I'd be fine with a one-mainloop-per-socket restriction, but we'd need to do some refactoring to make that happen as we currently in fact use multiple instances of selector in some situations (ConnectionFileDescriptor uses a SelectHelper internally and lldb-server adds the connection to its top level main loop).
|
In case it helps, here's a method to wait on a pipe using zero-length reads (inspired by the libdispatch implementation you found earlier). In case waiting on the pipe handle itself produces spurious wakeups, I think this should be a fairly reliable alternative. The actual implementation probably should handle the zero-length write like libdispatch, but retrying there should be fine as it should only happen if/when someone does a zero-length write. I'm hoping we can ignore PIPE_NOWAIT pipes since they're kind of deprecated. |
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.
This updates MainLoopWindows to support events for reading from a file and a socket type.
This unifies both handle types using
WaitForMultipleEventswhich can listen to both sockets and files for change events.This should allow us to unify how we handle watching files/pipes/sockets on Windows and Posix systems.