Skip to content

Factor out the network layer, and give it a window #2066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nicolasnoble wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Factor out the network layer, and give it a window #2066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c66f8e1
UvFifoListener: report bind and listen failures.
nicolasnoble Jul 28, 2026
55e8a50
UvFifo: optional readable notification.
nicolasnoble Jul 28, 2026
9b60dfc
UvFifo: make a failed connect actionable.
nicolasnoble Jul 28, 2026
0110676
Add a common base for network endpoints.
nicolasnoble Jul 28, 2026
b436c52
GDB server: move onto the common network endpoint.
nicolasnoble Jul 28, 2026
16224be
UvFifo: flush pending writes before closing.
nicolasnoble Jul 28, 2026
a4521ef
Build test objects under objs/ like everything else.
nicolasnoble Jul 28, 2026
b43f575
Web server: move onto the common network endpoint.
nicolasnoble Jul 28, 2026
acb1a02
SIO1: move both halves onto the common network endpoint.
nicolasnoble Jul 28, 2026
d5e9102
Add a Network window.
nicolasnoble Jul 29, 2026
9e52a38
Fix the CI failures on the network series.
nicolasnoble Jul 29, 2026
d123dd7
Link libuv and libcurl into the support test project.
nicolasnoble Jul 29, 2026
80c8fb5
Link the Windows system libraries into the support test project.
nicolasnoble Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,12 @@ VIXL_OBJECTS := $(addprefix objs/$(BUILD)/,$(patsubst %.cc,%.o,$(filter %.cc,$(V
$(IMGUI_OBJECTS): EXTRA_CPPFLAGS := $(IMGUI_CPPFLAGS)

TESTS_SRC := $(call rwildcard,tests/,*.cc)
TESTS := $(patsubst %.cc,%,$(TESTS_SRC))
TESTS_OBJECTS := $(addprefix objs/$(BUILD)/,$(patsubst %.cc,%.o,$(TESTS_SRC)))

DEPS += $(addprefix deps/$(BUILD)/,$(patsubst %.c,%.dep,$(filter %.c,$(SRCS))))
DEPS += $(addprefix deps/$(BUILD)/,$(patsubst %.cc,%.dep,$(filter %.cc,$(SRCS))))
DEPS += $(addprefix deps/$(BUILD)/,$(patsubst %.cpp,%.dep,$(filter %.cpp,$(SRCS))))
DEPS += $(addprefix deps/$(BUILD)/,$(patsubst %.cc,%.dep,$(TESTS_SRC)))

CP ?= cp
MKDIRP ?= mkdir -p
Expand Down Expand Up @@ -329,7 +330,7 @@ objs/$(BUILD)/gtest_main.o: third_party/googletest/googletest/src/gtest_main.cc
$(CXX) -O3 -g $(CXXFLAGS) -Ithird_party/googletest/googletest -Ithird_party/googletest/googletest/include -c third_party/googletest/googletest/src/gtest_main.cc -o objs/$(BUILD)/gtest_main.o

clean:
rm -f $(OBJECTS) $(TOOLS) $(TARGET) bins/$(BUILD)/$(TARGET) $(addprefix bins/$(BUILD)/,$(TOOLS)) $(DEPS) objs/$(BUILD)/gtest-all.o objs/$(BUILD)/gtest_main.o
rm -f $(OBJECTS) $(TESTS_OBJECTS) $(TOOLS) $(TARGET) bins/$(BUILD)/$(TARGET) $(addprefix bins/$(BUILD)/,$(TOOLS)) $(DEPS) objs/$(BUILD)/gtest-all.o objs/$(BUILD)/gtest_main.o
$(MAKE) -C third_party/luajit clean MACOSX_DEPLOYMENT_TARGET=$(MACOS_MIN_VERSION)

cleanall:
Expand All @@ -354,9 +355,9 @@ regen-i18n:
rm pcsx-src-list.txt
$(foreach l,$(LOCALES),$(call msgmerge,$(l)))

bins/$(BUILD)/pcsx-redux-tests: $(foreach t,$(TESTS),$(t).o) $(NONMAIN_OBJECTS) $(LIBS) objs/$(BUILD)/gtest-all.o objs/$(BUILD)/gtest_main.o
bins/$(BUILD)/pcsx-redux-tests: $(TESTS_OBJECTS) $(NONMAIN_OBJECTS) $(LIBS) objs/$(BUILD)/gtest-all.o objs/$(BUILD)/gtest_main.o
@$(MKDIRP) $(dir $@)
$(LD) -o bins/$(BUILD)/pcsx-redux-tests $(NONMAIN_OBJECTS) $(LIBS) objs/$(BUILD)/gtest-all.o objs/$(BUILD)/gtest_main.o $(foreach t,$(TESTS),$(t).o) -Ithird_party/googletest/googletest/include $(LDFLAGS)
$(LD) -o bins/$(BUILD)/pcsx-redux-tests $(NONMAIN_OBJECTS) $(LIBS) objs/$(BUILD)/gtest-all.o objs/$(BUILD)/gtest_main.o $(TESTS_OBJECTS) -Ithird_party/googletest/googletest/include $(LDFLAGS)

pcsx-redux-tests: check_submodules bins/$(BUILD)/pcsx-redux-tests
$(CP) bins/$(BUILD)/pcsx-redux-tests pcsx-redux-tests
Expand Down
127 changes: 80 additions & 47 deletions src/core/gdb-server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/***************************************************************************

Check notice on line 1 in src/core/gdb-server.cc

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ Getting better: Overall Code Complexity 

The mean cyclomatic complexity decreases from 8.20 to 8.18, threshold = 4

This file has many conditional statements (e.g. if, for, while) across its implementation, leading to
lower code health. Avoid adding more conditionals.

Check notice on line 1 in src/core/gdb-server.cc

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ No longer an issue: Primitive Obsession 

The ratio of primivite types in function arguments is no longer above the threshold
* Copyright (C) 2020 PCSX-Redux authors *
* *
* This program is free software; you can redistribute it and/or modify *
Expand Down Expand Up @@ -34,75 +34,108 @@

const char PCSX::GdbClient::toHex[] = "0123456789ABCDEF";

PCSX::GdbServer::GdbServer() : m_listener(g_system->m_eventBus) {
PCSX::GdbServer::GdbServer() : Network::Server("GDB Server"), m_listener(g_system->m_eventBus) {
m_listener.listen<Events::SettingsLoaded>([this](const auto& event) {
auto& args = g_system->getArgs();
auto& settings = g_emulator->settings.get<Emulator::SettingDebugSettings>();
if (settings.get<Emulator::DebugSettings::GdbServer>() && (m_serverStatus != SERVER_STARTED)) {
startServer(g_system->getLoop(), settings.get<Emulator::DebugSettings::GdbServerPort>());
if (settings.get<Emulator::DebugSettings::GdbServer>() && !isRunning()) {
start(g_system->getLoop(), settings.get<Emulator::DebugSettings::GdbServerPort>());
}
});
m_listener.listen<Events::Quitting>([this](const auto& event) {
if (m_serverStatus == SERVER_STARTED) stopServer();
if (isRunning()) stop();
});
}

void PCSX::GdbServer::stopServer() {
assert(m_serverStatus == SERVER_STARTED);
m_serverStatus = SERVER_STOPPING;
for (auto& client : m_clients) client.close();
uv_close(reinterpret_cast<uv_handle_t*>(&m_server), closeCB);
void PCSX::GdbServer::onConnection(IO<File> connection) {
m_clients.push_back(new GdbClient(connection, g_system->getLoop()));
}

void PCSX::GdbServer::startServer(uv_loop_t* loop, int port) {
assert(m_serverStatus == SERVER_STOPPED);

uv_tcp_init(loop, &m_server);
m_server.data = this;

struct sockaddr_in bindAddr;
int result = uv_ip4_addr("0.0.0.0", port, &bindAddr);
if (result != 0) {
uv_close(reinterpret_cast<uv_handle_t*>(&m_server), closeCB);
return;
void PCSX::GdbServer::onStopped() {
// Covers an orderly stop and a failed bind alike. Iterate defensively:
// closing a client eventually deletes it, which unlinks it from this list.
while (!m_clients.empty()) {
auto client = m_clients.begin();
client->close();
if (!m_clients.empty() && (m_clients.begin() == client)) m_clients.erase(client);
}
result = uv_tcp_bind(&m_server, reinterpret_cast<const sockaddr*>(&bindAddr), 0);
if (result != 0) {
uv_close(reinterpret_cast<uv_handle_t*>(&m_server), closeCB);
return;
}
result = uv_listen((uv_stream_t*)&m_server, 16, onNewConnectionTrampoline);
if (result != 0) {
uv_close(reinterpret_cast<uv_handle_t*>(&m_server), closeCB);
}

void PCSX::GdbClient::logOutgoing(const Slice& slice) {
if (!g_emulator->settings.get<Emulator::SettingDebugSettings>()
.get<Emulator::DebugSettings::GdbServerTrace>()) {
return;
}
m_serverStatus = SERVER_STARTED;
std::string msg(static_cast<const char*>(slice.data()), slice.size());
g_system->log(LogClass::GDB, "GDB <-- PCSX %s\n", msg.c_str());
}

void PCSX::GdbServer::closeCB(uv_handle_t* handle) {
GdbServer* self = static_cast<GdbServer*>(handle->data);
self->m_serverStatus = SERVER_STOPPED;
void PCSX::GdbClient::sendRaw(Slice&& raw) {
if (!m_connection || m_connection->isClosed()) return;
logOutgoing(raw);
m_connection->write(std::move(raw));
}

void PCSX::GdbServer::onNewConnectionTrampoline(uv_stream_t* handle, int status) {
GdbServer* self = static_cast<GdbServer*>(handle->data);
self->onNewConnection(status);
void PCSX::GdbClient::sendPacket(Slice&& payload) {
if (!m_connection || m_connection->isClosed()) return;
logOutgoing(payload);
uint8_t chksum = 0;
auto data = static_cast<const uint8_t*>(payload.data());
for (size_t i = 0; i < payload.size(); i++) chksum += data[i];
std::string framed;
framed.reserve(payload.size() + 4);
framed += '$';
framed.append(static_cast<const char*>(payload.data()), payload.size());
framed += '#';
framed += toHex[chksum >> 4];
framed += toHex[chksum & 0x0f];
Slice out;
out.acquire(std::move(framed));
m_connection->write(std::move(out));
}

void PCSX::GdbServer::onNewConnection(int status) {
if (status < 0) return;
GdbClient* client = new GdbClient(&m_server);
if (client->accept(&m_server)) {
m_clients.push_back(client);
} else {
delete client;
void PCSX::GdbClient::onReadable() {
uint8_t buffer[BUFFER_SIZE];
while (m_connection && (m_status == OPEN) && (m_connection->size() > 0)) {

Check warning on line 98 in src/core/gdb-server.cc

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Complex Conditional 

PCSX::GdbClient::onReadable has 1 complex conditionals with 2 branches, threshold = 2

A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple,
logical operators such as AND/OR. The more logical operators in an expression, the more severe the code
smell.
auto toRead = std::min(sizeof(buffer), m_connection->size());
auto got = m_connection->read(buffer, toRead);
if (got <= 0) break;
Slice slice;
slice.borrow(buffer, got);
processData(slice);
}
// eof() is closed-and-drained, so a peer that hung up mid-packet still gets
// everything it managed to send processed before we tear the client down.
if (m_connection && m_connection->eof()) close();
}

void PCSX::GdbClient::close() {
if (m_status != OPEN) return;
m_status = CLOSING;
if (m_connection) {
m_connection.asA<UvFifo>()->clearNotifier();
m_connection.reset();
}
// Deleting the client here would free the object the async callback is
// running inside of. Hand that off to the async's own close callback, which
// uv only runs once the handle is genuinely done.
auto context = m_asyncContext;
m_asyncContext = nullptr;
if (!context) {
delete this;
return;
}
uv_close(reinterpret_cast<uv_handle_t*>(&context->m_async), [](uv_handle_t* handle) {
auto context = reinterpret_cast<AsyncContext*>(handle);
delete context->m_client;
delete context;
});
}
Comment on lines +111 to 132

@coderabbitai coderabbitai Bot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

The GDB and Web clients now carry two verbatim copies of the same connection lifecycle. AsyncContext (async + owner back-pointer), the setNotifier/clearNotifier pairing, the OPEN/CLOSING state, the drain-until-empty onReadable() loop, the deferred delete inside the uv_close callback, and the onStopped() defensive-iteration teardown are byte-for-byte equivalent in both files. The header comment in src/core/gdb-server.h even notes the previous framing code was "duplicated verbatim in the web server" — this refactor moved the duplication rather than removing it. A small Network::Connection (or a CRTP base) next to Network::Server would own the async context, the notifier, and the deferred teardown, leaving each client with only its protocol-specific processData.

  • src/core/gdb-server.cc#L111-L132: hoist the AsyncContext + uv_close deferred-delete teardown into a shared helper and have GdbClient use it.
  • src/core/web-server.cc#L887-L911: replace WebClientImpl's identical close()/AsyncContext implementation with the same shared helper.
📍 Affects 2 files
  • src/core/gdb-server.cc#L111-L132 (this comment)
  • src/core/web-server.cc#L887-L911
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/gdb-server.cc` around lines 111 - 132, Extract the duplicated
connection lifecycle into a shared Network::Connection helper near
Network::Server, including AsyncContext ownership, notifier setup/clearing,
OPEN/CLOSING state handling, and deferred uv_close deletion. Update
GdbClient::close in src/core/gdb-server.cc:111-132 and WebClientImpl::close in
src/core/web-server.cc:887-911 to use that helper, leaving protocol-specific
processData behavior in each client.


PCSX::GdbClient::GdbClient(uv_tcp_t* srv) : m_listener(g_system->m_eventBus) {
m_loop = srv->loop;
uv_tcp_init(m_loop, &m_tcp);
m_tcp.data = this;
PCSX::GdbClient::GdbClient(IO<File> connection, uv_loop_t* loop) : m_listener(g_system->m_eventBus) {
m_loop = loop;
m_connection = connection;
m_asyncContext = new AsyncContext{{}, this};
m_connection.asA<UvFifo>()->setNotifier(loop, &m_asyncContext->m_async, [this]() { onReadable(); });
m_listener.listen<Events::ExecutionFlow::Run>([this](const auto& event) { m_exception = false; });
m_listener.listen<Events::ExecutionFlow::Pause>([this](const auto& event) {
m_exception = event.exception;
Expand Down
Loading
Loading