Factor out the network layer, and give it a window - #2066
Open
nicolasnoble wants to merge 13 commits into
Open
Conversation
The bind and listen both run on the uv worker thread, so a failure could not be returned from start(); all three error paths closed the server handle with an empty callback and told nobody. That left the listener in a state where a subsequent stop() closed the same handle a second time and tripped uv_close's !uv__is_closing assertion, which is reachable from the GUI by enabling a server on a busy port and switching it back off. Give the listener a status and the uv error code, and route the failure through the same nullptr callback an orderly shutdown uses, so consumers do not need a second teardown path and stop leaking their async handle when the bind fails. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Reads land in a lock-free queue on the uv worker thread and nothing tells the consumer, so the only way to notice traffic is to poll size() - which is what the SIO1 and ATCons bridges do off the counters. That is fine for them and not fine for anything latency sensitive, so a consumer can now hand over an async living on its own loop and be woken instead. The callback runs on that loop, and fires on arrival as well as on EOF. Installing a notifier on an already readable or already closed fifo fires it once immediately: an accepted connection starts reading the moment it exists on the worker thread, which is well before the consumer receives it through the listener's async, so otherwise the first packet of a connection could sit in the queue until a second one showed up. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
m_connecting was set in the constructor and cleared only on the success path, so a fifo that failed to connect reported isConnecting() forever. The SIO1 reconnect button is gated on !connecting() && fifoError(), which means it could never become clickable after the one failure it exists to recover from. Clear the flag on all three failure paths, and record the uv error code while we are here: a failed outgoing connection used to be a bare bool, which is enough to colour a status indicator red and not enough to say why. Wake any installed notifier too, so a consumer waiting on the fifo learns about the failure instead of waiting for a connection that is never coming. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Every server in the emulator hand-rolls the same uv_tcp_init / uv_ip4_addr / uv_tcp_bind / uv_listen sequence, declares its own three-state status enum with its own spelling, and has no way to express failure. Nothing reads any of those enums. Introduce Network::Endpoint with a single Status vocabulary and a last error, plus Network::Server on top of UvFifoListener and Network::Client on top of UvFifo. A concrete endpoint now says what to do with a connection and nothing else; connections arrive as IO<File>, so there is no per-server accept, read, or write-queue machinery to retype. Endpoints self-register, so the configuration UI can iterate them rather than hard-coding a row per service, and a new one gets a status indicator without touching the window. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The server drops its hand-rolled bind and listen, its private three-state status enum that nothing read, its dead m_gotError member, and its accept trampolines. A connection now arrives as an IO<File>. GdbClient loses the WriteRequest intrusive hash table and its 3-buffer scatter uv_writes along with the alloc and read trampolines and the manual close callback. A File has no scatter/gather, so a packet is framed into a single buffer and written as one Slice; GDB packets are small, and that is one allocation per packet against a hash table insert plus erase. Reads arrive through the fifo notifier rather than a poll, so there is no added latency on the stepping path. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Writes are queued to the uv worker thread, so returning from write() and the bytes leaving the machine are different moments, and uv_close cancels whatever is still queued. Closing right after writing therefore truncates the tail: with a peer that is not draining, a 64MB write followed by an immediate close delivered 2634240 bytes. Hand the teardown to whichever write completes last. The socket and the bookkeeping move into a control block held by both the fifo and every write in flight, so a fifo destroyed while writes are still queued cannot pull the state out from under the worker thread - the write path must never capture the fifo itself for that reason. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Test objects were compiled in place next to their sources by make's built-in suffix rule, which meant they had no header dependency tracking and were not removed by the clean target. Editing a header that a test includes therefore left the old object in place and linked it, and if the header changed a class layout the result was heap corruption at runtime rather than a build error. Route them through the same objs/ pattern rule as the rest of the tree so they pick up the existing dep generation, and stop littering tests/ with build output. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Same treatment as the GDB server. The server drops its hand-rolled bind and listen, its private status enum, its dead m_gotError member and its accept path; connections arrive as IO<File>. WebClientImpl loses the WriteRequest hash table, the alloc and read trampolines and the manual close callback. It also loses m_closeScheduled entirely: that existed to hold a connection open until every queued write had completed, because closing earlier truncated the response. The transport flushes on close now, so scheduleClose is just close. Verified against a running server: a 404 frames correctly, and the vram and ram endpoints return their full 1MB and 2MB rather than whatever fit in the socket buffer. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The server and the client each carried their own three-state enum, and the server set SERVER_STARTED before handing the bind to the listener on the other thread, so its status could never report a failure. Both now derive their state from the transport, which has one. The client also stops throwing std::runtime_error out of an ImGui checkbox callback when raw mode is selected: an unsupported configuration is not an exceptional condition, so it says so and leaves the endpoint alone. Adds onStarting and onStarted hooks to the endpoint bases, since both halves flip an emulator poll flag and hand the connection to SIO1 rather than keeping it themselves. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The network services were a flat run of checkboxes buried in the emulation configuration window, with nothing anywhere indicating whether a server was listening or a client connected. A failed bind left the checkbox ticked and said nothing at all. Move them into their own window, one section per service, each with a status indicator, the state in words, a restart button and the error string when there is one. The indicator is drawn rather than glyphed so it does not depend on the font. Also drops ImGuiInputTextFlags_CharsDecimal from the SIO1 client host field, which made it impossible to type a hostname into it. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change adds shared network endpoint lifecycle abstractions, migrates GDB, Web, and SIO1 networking, introduces a dedicated Network settings window, hardens ChangesNetwork lifecycle refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GUI
participant NetworkServer
participant UvFifoListener
participant WebClient
GUI->>NetworkServer: start(loop, port)
NetworkServer->>UvFifoListener: listen
UvFifoListener-->>NetworkServer: accepted connection
NetworkServer->>WebClient: onConnection(IO<File>)
WebClient->>WebClient: register readable notifier
WebClient-->>GUI: endpoint status and error state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
The window, with the GDB server and the web server deliberately pointed at the
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
tests/support/uvfifolistener.cc (1)
272-277: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value64 MiB payload is doubled by the write path.
UvFifo::write(const void*, size_t)copies intoinfo->slice, so this test peaks at ~128 MiB plus whatever the kernel buffers, with a 30 s deadline attached. A few MiB is already far beyond any socket buffer and keeps the writes genuinely pending, which is all the test needs.🤖 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 `@tests/support/uvfifolistener.cc` around lines 272 - 277, Reduce c_payloadSize in the pending-write test around UvFifo::write to a few MiB, keeping it large enough to exceed socket buffers and preserve ASSERT_GT(accepted->pendingWrites(), 0u). Avoid the unnecessary 64 MiB allocation and duplicated write-path memory.src/support/uvfile.cc (1)
867-881: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
clearNotifier()leavesm_notifyCbinstalled.
clearNotifier()only nullsm_notifyAsync; anuv_async_sendalready in flight will still run the old callback on the consumer loop. Consumers likeWebClientImpl::close()clear the notifier and then destroy the objects the callback captured, relying onuv_closeof their async landing first. Clearingm_notifyCbtoo (from the consumer loop, same thread that runs the async callback) would make that safe by construction rather than by ordering luck.🤖 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/support/uvfile.cc` around lines 867 - 881, Update clearNotifier() to also clear m_notifyCb on the consumer loop, alongside resetting m_notifyAsync, so any already queued uv_async callback cannot invoke a destroyed consumer callback. Preserve setNotifier()’s callback installation and notification behavior.tests/support/network.cc (1)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixed ports plus wall-clock pumping makes these tests order- and load-sensitive.
pump()is a fixed-duration sleep loop, so a loaded CI runner can miss the transition and fail with no useful signal; the port constants also collide if two builds run concurrently on the same host. Consider polling for the expected status with a deadline (asConnectFailureIsVisiblealready does) and binding port 0 where the port value is not itself under test.🤖 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 `@tests/support/network.cc` around lines 35 - 46, The fixed test ports and fixed-duration pump make network tests susceptible to concurrent-build collisions and loaded-runner timing failures. Update the relevant tests in tests/support/network.cc to bind port 0 whenever the specific port number is not under test, and replace fixed-duration pump usage with deadline-based polling for the expected status, following the existing ConnectFailureIsVisible pattern.src/gui/widgets/network.h (1)
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment:
drawStatusreturnsvoid, not a width.📝 Proposed fix
- // Grey stopped, amber connecting, green up, red failed. Returns the width - // consumed so the rows line up. + // Grey stopped, amber connecting, green up, red failed. Advances the + // cursor past the bullet so the rows line up. void drawStatus(const PCSX::Network::Endpoint* endpoint);🤖 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/gui/widgets/network.h` around lines 42 - 44, Update the comment above drawStatus to accurately state that the method returns void and remove the claim that it returns the consumed width, while preserving the status-color description.src/core/gdb-server.cc (1)
96-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the EOF check with
m_statusfor consistency with the web client.
processData()can callclose()mid-drain, which resetsm_connection, so the current check is safe today only because of that side effect. The web server's equivalent (src/core/web-server.ccLine 1071) additionally testsm_status == OPEN; mirroring it makes the invariant explicit rather than incidental.🤖 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 96 - 109, Update the EOF cleanup condition in GdbClient::onReadable() to require both m_status == OPEN and m_connection->eof() before calling close(). Preserve the existing null-connection guard and ensure the check mirrors the web client’s explicit open-status invariant.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/core/gdb-server.cc`:
- Around line 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.
In `@src/core/sio1-server.cc`:
- Around line 75-86: Make the Raw-mode refusal in SIO1Client::onStarting abort
the connection startup rather than merely returning. Propagate a veto through
the Network::Client::start() path so it exits before creating the UvFifo,
assigning m_connection, or calling onStarted(); ensure the endpoint does not
report Running and avoid leaving m_sio1Mode or polling state partially
configured.
- Around line 56-59: Update SIO1Server::onStopped() and the corresponding
SIO1Client::onStopped() so stopping one endpoint does not directly clear the
shared m_pollSIO1 flag or stop/reset the global SIO1 connection while the other
endpoint remains active. Track endpoint activity independently and only clear or
reset shared SIO1 state when no FIFO/client/server endpoint is still using the
session.
In `@src/gui/widgets/network.cc`:
- Around line 96-232: Refactor Network::draw into per-section helpers named
drawGdb, drawWeb, and drawSIO1, keeping each section’s existing controls and
changed-state behavior intact. Extract the repeated enable-checkbox start/stop
logic into a shared helper that accepts the endpoint, enabled setting, event
loop, and relevant port (and client host where needed). Have draw only render
the section headers, invoke the helpers, and return the accumulated changed
result while preserving Begin/End handling.
- Around line 63-68: Update drawEndpointHeader to render Endpoint::name() with
its explicit string_view length instead of passing name().data() to a
NUL-terminated %s formatter. Remove the unused restarted return-value plumbing,
or propagate and fold it into the existing changed state at every
drawEndpointHeader call site (Lines 107, 153, 173, and 191).
In `@src/support/network.cc`:
- Around line 72-79: Update Server::start to return when status() is either
Status::Running or Status::Starting, preventing re-entry while listener setup is
in flight. Preserve the existing initialization and listener.start flow for
other states.
- Around line 98-110: Update Server::stop() so the already-down path settles
teardown without invoking onListenerEvent(nullptr) a second time after a failed
bind has already delivered it. Preserve the existing listener.stop() flow for
Listening and Starting states, while ensuring restart() and the Quitting handler
do not trigger duplicate onStopped() notifications.
In `@src/support/uvfile.cc`:
- Around line 898-909: Bound the deferred shutdown in UvFifo::closeInternal by
arming a worker-loop uv_timer_t when m_closeRequested is set, while retaining
the immediate closeNow() path when m_pending is already zero. Have the timer
invoke closeNow() if pending writes do not drain before the timeout, and ensure
the timer is stopped and cleaned up when writes finish or closeNow() runs so
socket, Info, and Control resources are reclaimed without double-closing.
- Around line 1005-1015: Update UvFifoListener::failed() and stop() so terminal
status is established before invoking uv_close, and ensure each path checks
uv_is_closing (or otherwise atomically prevents repeated closure). Preserve the
existing wake-up behavior while preventing queued failed/stop requests or
multiple stop() calls in the same worker drain from closing m_server more than
once.
- Around line 952-966: Handle synchronous uv_write failures in both the
raw-pointer and Slice&& overloads of the write operation at
src/support/uvfile.cc lines 952-966 and 987-1002: capture the return value, and
when it is negative, delete the allocated Info and call
control->writeCompleted() before returning. Keep the existing completion
callback behavior for successful submissions.
In `@tests/support/uvfifolistener.cc`:
- Around line 86-94: Update the cleanup in both affected tests around
listener.stop() and the final uv_run call: explicitly close the stack-allocated
uv_async_t handle, pump the loop until its close callback completes, then assert
uv_loop_close(&loop) succeeds. Preserve the existing status and callback
assertions while ensuring no handle remains open when the loop is closed.
---
Nitpick comments:
In `@src/core/gdb-server.cc`:
- Around line 96-109: Update the EOF cleanup condition in
GdbClient::onReadable() to require both m_status == OPEN and m_connection->eof()
before calling close(). Preserve the existing null-connection guard and ensure
the check mirrors the web client’s explicit open-status invariant.
In `@src/gui/widgets/network.h`:
- Around line 42-44: Update the comment above drawStatus to accurately state
that the method returns void and remove the claim that it returns the consumed
width, while preserving the status-color description.
In `@src/support/uvfile.cc`:
- Around line 867-881: Update clearNotifier() to also clear m_notifyCb on the
consumer loop, alongside resetting m_notifyAsync, so any already queued uv_async
callback cannot invoke a destroyed consumer callback. Preserve setNotifier()’s
callback installation and notification behavior.
In `@tests/support/network.cc`:
- Around line 35-46: The fixed test ports and fixed-duration pump make network
tests susceptible to concurrent-build collisions and loaded-runner timing
failures. Update the relevant tests in tests/support/network.cc to bind port 0
whenever the specific port number is not under test, and replace fixed-duration
pump usage with deadline-based polling for the expected status, following the
existing ConnectFailureIsVisible pattern.
In `@tests/support/uvfifolistener.cc`:
- Around line 272-277: Reduce c_payloadSize in the pending-write test around
UvFifo::write to a few MiB, keeping it large enough to exceed socket buffers and
preserve ASSERT_GT(accepted->pendingWrites(), 0u). Avoid the unnecessary 64 MiB
allocation and duplicated write-path memory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 597330a6-f100-4c78-8b0a-6f1b5b9168fc
📒 Files selected for processing (17)
Makefilesrc/core/gdb-server.ccsrc/core/gdb-server.hsrc/core/sio1-server.ccsrc/core/sio1-server.hsrc/core/web-server.ccsrc/core/web-server.hsrc/gui/gui.ccsrc/gui/gui.hsrc/gui/widgets/network.ccsrc/gui/widgets/network.hsrc/support/network.ccsrc/support/network.hsrc/support/uvfile.ccsrc/support/uvfile.htests/support/network.cctests/support/uvfifolistener.cc
Comment on lines
+111
to
132
| 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; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 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 theAsyncContext+uv_closedeferred-delete teardown into a shared helper and haveGdbClientuse it.src/core/web-server.cc#L887-L911: replaceWebClientImpl's identicalclose()/AsyncContextimplementation 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.
Comment on lines
+56
to
59
| void PCSX::SIO1Server::onStopped() { | ||
| g_emulator->m_counters->m_pollSIO1 = false; | ||
| m_fifoListener.stop(); | ||
| g_emulator->m_sio1->stopSIO1Connection(); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'm_pollSIO1|stopSIO1Connection' --type=cpp --type=cc -g '!**/third_party/**'Repository: grumpycoders/pcsx-redux
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching sio1 =="
fd -i 'sio1' . | sed -n '1,80p'
echo "== occurrences in tracked C/C++ files =="
python3 - <<'PY'
import subprocess, pathlib
res = subprocess.run(['git','ls-files'], text=True, check=True, stdout=subprocess.PIPE)
for p in res.stdout.splitlines():
s=pathlib.Path(p)
if s.suffix.lower() in {'.cc','.cpp','.h','.hpp','.c', '.inl'}:
pass
else:
continue
txt=pathlib.Path(p).read_text(errors='ignore')
for needle in ['m_pollSIO1','stopSIO1Connection']:
if needle in txt:
lines=txt.splitlines()
print(f"\n--- {p} : {needle} ---")
for i,l in enumerate(lines,1):
if needle in l:
lo=max(1,i-4); hi=min(len(lines),i+5)
for n in range(lo,hi+1):
print(f"{n:4}: {lines[n-1]}")
break
PY
echo "== outline candidates =="
for f in $(fd -i 'sio1-server\.cc|sio1-client\.cc|.*counter.*|sio.*' . | sed -n '1,40p'); do
echo "--- outline $f ---"
ast-grep outline "$f" --view compact 2>/dev/null | sed -n '1,120p' || true
doneRepository: grumpycoders/pcsx-redux
Length of output: 4446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Git diff stat/name-status =="
git diff --stat | sed -n '1,120p'
echo
git diff --name-status | sed -n '1,160p'
echo "== sio1 server/client diff hunks (contextual, limited) =="
for f in $(git diff --name-only | rg -i 'sio1|server|client|counter' | sed -n '1,120p'); do
echo "--- diff $f ---"
git diff -- "$f" | sed -n '1,240p' | head -n 240
doneRepository: grumpycoders/pcsx-redux
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/core/sio1-server.cc =="
cat -n src/core/sio1-server.cc
echo
echo "== src/gui/widgets/sio1.cc =="
cat -n src/gui/widgets/sio1.cc
echo
echo "== focused occurrences in widgets/core headers =="
python3 - <<'PY'
import pathlib
for p in pathlib.Path('.').rglob('*'):
if str(p).startswith('./third_party/'):
continue
s = p.suffix.lower()
if s in {'.cc','.cpp','.cc','.h','.hpp'}:
try:
txt = p.read_text(errors='ignore')
except PermissionError:
continue
for needle in ['stopSIO1Connection','m_pollSIO1','SIO1Server','SIO1Client','setFifo','startSIO1','stopSIO1']:
if needle in txt:
print(f"\n--- {p} : {needle} ---")
lines=txt.splitlines()
for i,l in enumerate(lines,1):
if needle in l:
lo=max(1,i-4); hi=min(len(lines),i+5)
for n in range(lo,hi+1):
print(f"{n:4}: {lines[n-1]}")
break
PY
echo
echo "== read-only structural state for SIO1 control names =="
python3 - <<'PY'
import re, pathlib
needles = {
"stopSIO1Connection": re.compile(r"void\s+(?:PCSX::)?SIO1::stopSIO1Connection\s*\("),
"m_pollSIO1": re.compile(r"\bm_pollSIO1\b"),
"setFifo": re.compile(r"\bsetFifo\b"),
}
for key, pat in needles.items():
hits=[]
for p in pathlib.Path('.').rglob('*'):
if str(p).startswith('./third_party/'):
continue
if p.suffix.lower() not in {'.cc','.cpp','.cc','.h','.hpp'}:
continue
try:
txt=p.read_text(errors='ignore')
except PermissionError:
continue
if key in txt:
hits.append(str(p))
print(f"{key}: {sorted(hits)}")
PYRepository: grumpycoders/pcsx-redux
Length of output: 31573
Don’t clear SIO1 global state from each endpoint stop
Both SIO1Server::onStopped() and SIO1Client::onStopped() write the same m_counters->m_pollSIO1 flag and call g_emulator->m_sio1->stopSIO1Connection()/reset(). In src/gui/widgets/network.cc these are exposed as separate checkboxes, so disabling one endpoint while the other is using the session can silently break the remaining endpoint. Track SIO1 state per active FIFO/client/server state instead of letting the stopped half clear/reset what the still-running half needs.
🤖 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/sio1-server.cc` around lines 56 - 59, Update SIO1Server::onStopped()
and the corresponding SIO1Client::onStopped() so stopping one endpoint does not
directly clear the shared m_pollSIO1 flag or stop/reset the global SIO1
connection while the other endpoint remains active. Track endpoint activity
independently and only clear or reset shared SIO1 state when no
FIFO/client/server endpoint is still using the session.
Comment on lines
+75
to
86
| void PCSX::SIO1Client::onStarting() { | ||
| auto mode = currentSIO1Mode(); | ||
| g_emulator->m_sio1->m_sio1Mode = mode; | ||
| if (mode == SIO1::SIO1Mode::Raw) { | ||
| // This used to throw std::runtime_error straight out of an ImGui | ||
| // checkbox callback. It is a configuration the client does not support, | ||
| // not an exceptional condition, so say so and leave the endpoint alone. | ||
| g_system->printf("%s", _("SIO1 client does not support raw mode\n")); | ||
| return; | ||
| } | ||
| g_emulator->m_counters->m_pollSIO1 = true; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Raw mode no longer aborts the client connection — it just skips polling.
onStarting() is invoked by Client::start() before the fifo is created (src/support/network.cc:130-141); returning early does not prevent start() from constructing the UvFifo, assigning m_connection, and calling onStarted(), which hands the fifo to SIO1. So in Raw mode the client still dials out and the endpoint reports Running, but m_pollSIO1 is never enabled — a half-configured state, and m_sio1Mode has already been set to Raw on Line 77. Replacing the throw with a message is right; the refusal needs to actually refuse.
🐛 Suggested approach
Give the base class a way to veto the start (e.g. bool onStarting() returning false to abort, with start() bailing out before creating the fifo), or check the mode in the caller/SettingsLoaded handler before calling start().
void PCSX::SIO1Client::onStarting() {
auto mode = currentSIO1Mode();
- g_emulator->m_sio1->m_sio1Mode = mode;
if (mode == SIO1::SIO1Mode::Raw) {
g_system->printf("%s", _("SIO1 client does not support raw mode\n"));
return;
}
+ g_emulator->m_sio1->m_sio1Mode = mode;
g_emulator->m_counters->m_pollSIO1 = true;
}(plus an actual abort path in Network::Client::start())
🤖 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/sio1-server.cc` around lines 75 - 86, Make the Raw-mode refusal in
SIO1Client::onStarting abort the connection startup rather than merely
returning. Propagate a veto through the Network::Client::start() path so it
exits before creating the UvFifo, assigning m_connection, or calling
onStarted(); ensure the endpoint does not report Running and avoid leaving
m_sio1Mode or polling state partially configured.
Comment on lines
+63
to
+68
| bool PCSX::Widgets::Network::drawEndpointHeader(PCSX::Network::Endpoint* endpoint, bool enabled) { | ||
| bool restarted = false; | ||
| drawStatus(endpoint); | ||
| ImGui::Text("%s", endpoint->name().data()); | ||
| ImGui::SameLine(); | ||
| ImGui::TextDisabled("(%s)", PCSX::Network::toString(endpoint->status())); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
%s on a std::string_view's data() is not NUL-safe, and the return value is never used.
Endpoint::name() returns a string_view; .data() only happens to be NUL-terminated because it currently views a std::string member. Use the length-bounded form. Separately, every call site (Lines 107, 153, 173, 191) discards the bool returned by drawEndpointHeader, so the restarted plumbing is currently dead — either drop it or fold it into changed.
🛡️ Proposed fix
- ImGui::Text("%s", endpoint->name().data());
+ const auto name = endpoint->name();
+ ImGui::Text("%.*s", static_cast<int>(name.size()), name.data());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool PCSX::Widgets::Network::drawEndpointHeader(PCSX::Network::Endpoint* endpoint, bool enabled) { | |
| bool restarted = false; | |
| drawStatus(endpoint); | |
| ImGui::Text("%s", endpoint->name().data()); | |
| ImGui::SameLine(); | |
| ImGui::TextDisabled("(%s)", PCSX::Network::toString(endpoint->status())); | |
| bool PCSX::Widgets::Network::drawEndpointHeader(PCSX::Network::Endpoint* endpoint, bool enabled) { | |
| bool restarted = false; | |
| drawStatus(endpoint); | |
| const auto name = endpoint->name(); | |
| ImGui::Text("%.*s", static_cast<int>(name.size()), name.data()); | |
| ImGui::SameLine(); | |
| ImGui::TextDisabled("(%s)", PCSX::Network::toString(endpoint->status())); |
🤖 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/gui/widgets/network.cc` around lines 63 - 68, Update drawEndpointHeader
to render Endpoint::name() with its explicit string_view length instead of
passing name().data() to a NUL-terminated %s formatter. Remove the unused
restarted return-value plumbing, or propagate and fold it into the existing
changed state at every drawEndpointHeader call site (Lines 107, 153, 173, and
191).
Comment on lines
+96
to
+232
| bool PCSX::Widgets::Network::draw(GUI* gui, const char* title) { | ||
| if (!ImGui::Begin(title, &m_show)) { | ||
| ImGui::End(); | ||
| return false; | ||
| } | ||
|
|
||
| bool changed = false; | ||
| auto& debugSettings = g_emulator->settings.get<Emulator::SettingDebugSettings>(); | ||
|
|
||
| // -- GDB server -- | ||
| if (ImGui::CollapsingHeader(_("GDB Server"), ImGuiTreeNodeFlags_DefaultOpen)) { | ||
| drawEndpointHeader(g_emulator->m_gdbServer.get(), debugSettings.get<Emulator::DebugSettings::GdbServer>()); | ||
| if (ImGui::Checkbox(_("Enable GDB Server"), &debugSettings.get<Emulator::DebugSettings::GdbServer>().value)) { | ||
| changed = true; | ||
| if (debugSettings.get<Emulator::DebugSettings::GdbServer>()) { | ||
| g_emulator->m_gdbServer->start(g_system->getLoop(), | ||
| debugSettings.get<Emulator::DebugSettings::GdbServerPort>()); | ||
| } else { | ||
| g_emulator->m_gdbServer->stop(); | ||
| } | ||
| } | ||
| ImGuiHelpers::ShowHelpMarker(_(R"(This will activate a gdb-server that you can | ||
| connect to with any gdb-remote compliant client. | ||
| You also need to enable the debugger.)")); | ||
| changed |= | ||
| ImGui::InputInt(_("GDB Server Port"), &debugSettings.get<Emulator::DebugSettings::GdbServerPort>().value); | ||
| ImGuiHelpers::ShowHelpMarker(_(R"(A port change only takes effect on the | ||
| next restart of the server.)")); | ||
| changed |= | ||
| ImGui::Checkbox(_("GDB send manifest"), &debugSettings.get<Emulator::DebugSettings::GdbManifest>().value); | ||
| ImGuiHelpers::ShowHelpMarker(_(R"(Enables sending the processor's manifest | ||
| from the gdb server. Keep this enabled, unless | ||
| you want to connect IDA to this server, as it | ||
| has a bug in its manifest parser.)")); | ||
| changed |= | ||
| ImGui::Checkbox(_("GDB Server Trace"), &debugSettings.get<Emulator::DebugSettings::GdbServerTrace>().value); | ||
| ImGuiHelpers::ShowHelpMarker(_(R"(The GDB server will start tracing its | ||
| protocol into the logs, which can be helpful to debug | ||
| the gdb server system itself.)")); | ||
| auto& currentGdbLog = debugSettings.get<Emulator::DebugSettings::GdbLogSetting>().value; | ||
| auto currentGdbLogName = magic_enum::enum_name(currentGdbLog); | ||
| if (ImGui::BeginCombo(_("PCSX Logs to GDB"), currentGdbLogName.data())) { | ||
| for (auto v : magic_enum::enum_values<Emulator::DebugSettings::GdbLog>()) { | ||
| bool selected = (v == currentGdbLog); | ||
| auto name = magic_enum::enum_name(v); | ||
| if (ImGui::Selectable(name.data(), selected)) { | ||
| currentGdbLog = v; | ||
| changed = true; | ||
| } | ||
| if (selected) ImGui::SetItemDefaultFocus(); | ||
| } | ||
| ImGui::EndCombo(); | ||
| } | ||
| } | ||
|
|
||
| // -- Web server -- | ||
| if (ImGui::CollapsingHeader(_("Web Server"), ImGuiTreeNodeFlags_DefaultOpen)) { | ||
| drawEndpointHeader(g_emulator->m_webServer.get(), debugSettings.get<Emulator::DebugSettings::WebServer>()); | ||
| if (ImGui::Checkbox(_("Enable Web Server"), &debugSettings.get<Emulator::DebugSettings::WebServer>().value)) { | ||
| changed = true; | ||
| if (debugSettings.get<Emulator::DebugSettings::WebServer>()) { | ||
| g_emulator->m_webServer->start(g_system->getLoop(), | ||
| debugSettings.get<Emulator::DebugSettings::WebServerPort>()); | ||
| } else { | ||
| g_emulator->m_webServer->stop(); | ||
| } | ||
| } | ||
| ImGuiHelpers::ShowHelpMarker(_(R"(This will activate a web-server, that you can | ||
| query using a REST api. See the wiki for details.)")); | ||
| changed |= | ||
| ImGui::InputInt(_("Web Server Port"), &debugSettings.get<Emulator::DebugSettings::WebServerPort>().value); | ||
| ImGuiHelpers::ShowHelpMarker(_(R"(A port change only takes effect on the | ||
| next restart of the server.)")); | ||
| } | ||
|
|
||
| // -- SIO1 -- | ||
| if (ImGui::CollapsingHeader(_("SIO1"), ImGuiTreeNodeFlags_DefaultOpen)) { | ||
| drawEndpointHeader(g_emulator->m_sio1Server.get(), debugSettings.get<Emulator::DebugSettings::SIO1Server>()); | ||
| if (ImGui::Checkbox(_("Enable SIO1 Server"), &debugSettings.get<Emulator::DebugSettings::SIO1Server>().value)) { | ||
| changed = true; | ||
| if (debugSettings.get<Emulator::DebugSettings::SIO1Server>()) { | ||
| g_emulator->m_sio1Server->start(g_system->getLoop(), | ||
| debugSettings.get<Emulator::DebugSettings::SIO1ServerPort>()); | ||
| } else { | ||
| g_emulator->m_sio1Server->stop(); | ||
| } | ||
| } | ||
| ImGuiHelpers::ShowHelpMarker(_(R"(This will activate a tcp server, that will | ||
| relay information between tcp and sio1. | ||
| See the wiki for details.)")); | ||
| changed |= ImGui::InputInt(_("SIO1 Server Port"), | ||
| &debugSettings.get<Emulator::DebugSettings::SIO1ServerPort>().value); | ||
|
|
||
| ImGui::Separator(); | ||
|
|
||
| drawEndpointHeader(g_emulator->m_sio1Client.get(), debugSettings.get<Emulator::DebugSettings::SIO1Client>()); | ||
| if (ImGui::Checkbox(_("Enable SIO1 Client"), &debugSettings.get<Emulator::DebugSettings::SIO1Client>().value)) { | ||
| changed = true; | ||
| if (debugSettings.get<Emulator::DebugSettings::SIO1Client>()) { | ||
| g_emulator->m_sio1Client->start( | ||
| g_system->getLoop(), | ||
| std::string_view(debugSettings.get<Emulator::DebugSettings::SIO1ClientHost>().value), | ||
| debugSettings.get<Emulator::DebugSettings::SIO1ClientPort>()); | ||
| } else { | ||
| g_emulator->m_sio1Client->stop(); | ||
| } | ||
| } | ||
| // This used to be flagged CharsDecimal, so a hostname could not be | ||
| // typed into the hostname field. | ||
| changed |= ImGui::InputText(_("SIO1 Client Host"), | ||
| &debugSettings.get<Emulator::DebugSettings::SIO1ClientHost>().value); | ||
| changed |= ImGui::InputInt(_("SIO1 Client Port"), | ||
| &debugSettings.get<Emulator::DebugSettings::SIO1ClientPort>().value); | ||
|
|
||
| if (ImGui::Button(_("Reset SIO"))) { | ||
| g_emulator->m_sio1->reset(); | ||
| } | ||
|
|
||
| auto& currentSIO1Mode = debugSettings.get<Emulator::DebugSettings::SIO1ModeSetting>().value; | ||
| auto currentSIO1Name = magic_enum::enum_name(currentSIO1Mode); | ||
| if (ImGui::BeginCombo(_("SIO1Mode"), currentSIO1Name.data())) { | ||
| for (auto v : magic_enum::enum_values<Emulator::DebugSettings::SIO1Mode>()) { | ||
| bool selected = (v == currentSIO1Mode); | ||
| auto name = magic_enum::enum_name(v); | ||
| if (ImGui::Selectable(name.data(), selected)) { | ||
| currentSIO1Mode = v; | ||
| changed = true; | ||
| } | ||
| if (selected) ImGui::SetItemDefaultFocus(); | ||
| } | ||
| ImGui::EndCombo(); | ||
| } | ||
| } | ||
|
|
||
| ImGui::End(); | ||
| return changed; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
CI gate is failing on draw() — split it into per-endpoint helpers.
CodeScene's quality gate fails the build here: cyclomatic complexity 22 (threshold 9), 6 bumpy-road blocks (threshold 2), nesting depth 4. The function is three near-identical blocks (header, enable checkbox with start/stop, port input, extras). Extracting a helper for the recurring "checkbox toggles start/stop with this port" shape, plus one function per service section (drawGdb, drawWeb, drawSIO1), removes both the duplication and the gate failure.
♻️ Sketch of the shared toggle helper
+// Same shape for every server: a checkbox that starts on tick and stops on
+// untick, using the port the endpoint is configured with.
+template <typename PortSetting, typename EnableSetting>
+static bool drawServerToggle(const char* label, PCSX::Network::Server* server, EnableSetting& enable,
+ PortSetting& port) {
+ if (!ImGui::Checkbox(label, &enable.value)) return false;
+ if (enable) {
+ server->start(PCSX::g_system->getLoop(), port);
+ } else {
+ server->stop();
+ }
+ return true;
+}🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 186-186: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING64'
(syntaxError)
[error] 210-210: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING1024'
(syntaxError)
[error] 182-182: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING32'
(syntaxError)
[error] 191-191: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING128'
(syntaxError)
[error] 197-197: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING256'
(syntaxError)
[error] 206-206: failed to expand 'TYPESTRING', Invalid ## usage when expanding 'TYPESTRING512'
(syntaxError)
🤖 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/gui/widgets/network.cc` around lines 96 - 232, Refactor Network::draw
into per-section helpers named drawGdb, drawWeb, and drawSIO1, keeping each
section’s existing controls and changed-state behavior intact. Extract the
repeated enable-checkbox start/stop logic into a shared helper that accepts the
endpoint, enabled setting, event loop, and relevant port (and client host where
needed). Have draw only render the section headers, invoke the helpers, and
return the accumulated changed result while preserving Begin/End handling.
Sources: Linters/SAST tools, Pipeline failures
Comment on lines
+98
to
+110
| void Server::stop() { | ||
| auto listenerStatus = m_listener.status(); | ||
| if ((listenerStatus == UvFifoListener::Status::Listening) || | ||
| (listenerStatus == UvFifoListener::Status::Starting)) { | ||
| m_listener.stop(); | ||
| return; // onListenerEvent(nullptr) finishes the teardown | ||
| } | ||
| // Already down - a failed bind has torn itself down and delivered its | ||
| // nullptr already, so nothing further is going to call back. Settle here | ||
| // rather than waiting for an event that will never arrive, which is what | ||
| // would strand a pending restart. | ||
| onListenerEvent(nullptr); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
stop() after a failed bind delivers a second onStopped().
A failed listener has already driven onListenerEvent(nullptr) once (failed() enqueues the nullptr), so onStopped() fired then. Calling stop() afterwards — which restart() always does, and which the Quitting handler can do — runs it again. That contradicts the "told once and only once" contract asserted in tests/support/network.cc line 144. Only the settle step needs to run on this path.
♻️ Suggested shape
// Already down - a failed bind has torn itself down and delivered its
// nullptr already, so nothing further is going to call back. Settle here
// rather than waiting for an event that will never arrive, which is what
// would strand a pending restart.
- onListenerEvent(nullptr);
+ settled();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void Server::stop() { | |
| auto listenerStatus = m_listener.status(); | |
| if ((listenerStatus == UvFifoListener::Status::Listening) || | |
| (listenerStatus == UvFifoListener::Status::Starting)) { | |
| m_listener.stop(); | |
| return; // onListenerEvent(nullptr) finishes the teardown | |
| } | |
| // Already down - a failed bind has torn itself down and delivered its | |
| // nullptr already, so nothing further is going to call back. Settle here | |
| // rather than waiting for an event that will never arrive, which is what | |
| // would strand a pending restart. | |
| onListenerEvent(nullptr); | |
| } | |
| void Server::stop() { | |
| auto listenerStatus = m_listener.status(); | |
| if ((listenerStatus == UvFifoListener::Status::Listening) || | |
| (listenerStatus == UvFifoListener::Status::Starting)) { | |
| m_listener.stop(); | |
| return; // onListenerEvent(nullptr) finishes the teardown | |
| } | |
| // Already down - a failed bind has torn itself down and delivered its | |
| // nullptr already, so nothing further is going to call back. Settle here | |
| // rather than waiting for an event that will never arrive, which is what | |
| // would strand a pending restart. | |
| settled(); | |
| } |
🤖 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/support/network.cc` around lines 98 - 110, Update Server::stop() so the
already-down path settles teardown without invoking onListenerEvent(nullptr) a
second time after a failed bind has already delivered it. Preserve the existing
listener.stop() flow for Listening and Starting states, while ensuring restart()
and the Quitting handler do not trigger duplicate onStopped() notifications.
Comment on lines
898
to
909
| void PCSX::UvFifo::closeInternal() { | ||
| m_closed.store(true); | ||
| request([tcp = m_tcp](uv_loop_t *loop) { | ||
| if (!tcp) return; | ||
| uv_close(reinterpret_cast<uv_handle_t *>(tcp), [](uv_handle_t *handle) { | ||
| auto tcp = reinterpret_cast<uv_tcp_t *>(handle); | ||
| delete tcp; | ||
| }); | ||
| request([control = m_control](uv_loop_t *loop) { | ||
| // Graceful: uv_close cancels pending uv_writes, so tearing down here | ||
| // while writes are still in flight truncates them - for an HTTP | ||
| // response that is a silently short reply. Hand the teardown to | ||
| // whichever write finishes last instead. m_closeRequested and m_tcp are | ||
| // touched only on the worker thread, so the two cannot race. | ||
| control->m_closeRequested = true; | ||
| if (control->m_pending.load(std::memory_order_acquire) == 0) control->closeNow(); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Deferred close can never complete against a peer that stops reading.
Teardown is now handed to the last write to finish, but a peer that never drains keeps m_pending above zero indefinitely: the socket, the Info allocations, and the Control block stay alive with no timeout or upper bound. Previously uv_close reclaimed them (at the cost of truncation). For the web server this is a remote client's choice, so it is worth bounding — e.g. arm a uv_timer_t on the worker loop when m_closeRequested is set and call closeNow() if the writes have not drained by then.
🤖 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/support/uvfile.cc` around lines 898 - 909, Bound the deferred shutdown in
UvFifo::closeInternal by arming a worker-loop uv_timer_t when m_closeRequested
is set, while retaining the immediate closeNow() path when m_pending is already
zero. Have the timer invoke closeNow() if pending writes do not drain before the
timeout, and ensure the timer is stopped and cleaned up when writes finish or
closeNow() runs so socket, Info, and Control resources are reclaimed without
double-closing.
Comment on lines
+952
to
+966
| request([control = m_control, info](auto loop) { | ||
| if (!control->m_tcp) { | ||
| delete info; | ||
| }); | ||
| control->writeCompleted(); | ||
| return; | ||
| } | ||
| info->control = control; | ||
| info->buf.base = reinterpret_cast<decltype(info->buf.base)>(const_cast<void *>(info->slice.data())); | ||
| uv_write(&info->req, reinterpret_cast<uv_stream_t *>(control->m_tcp), &info->buf, 1, | ||
| [](uv_write_t *req, int status) { | ||
| auto info = reinterpret_cast<Info *>(req->data); | ||
| auto control = info->control; | ||
| delete info; | ||
| control->writeCompleted(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unchecked uv_write return leaks the request and permanently pins m_pending above zero. When uv_write fails synchronously (returns < 0, e.g. the stream is already shutting down) libuv never invokes the completion callback, so neither overload deletes info nor calls control->writeCompleted(). With the new graceful close that also means m_closeRequested never reaches zero pending and the socket is never torn down.
src/support/uvfile.cc#L952-L966: capture theuv_writeresult in the raw-pointer overload and, on failure,delete info; control->writeCompleted();before returning.src/support/uvfile.cc#L987-L1002: apply the identical failure handling in theSlice&&overload.
🛡️ Proposed fix (raw-pointer overload; mirror in the `Slice&&` one)
- uv_write(&info->req, reinterpret_cast<uv_stream_t *>(control->m_tcp), &info->buf, 1,
- [](uv_write_t *req, int status) {
- auto info = reinterpret_cast<Info *>(req->data);
- auto control = info->control;
- delete info;
- control->writeCompleted();
- });
+ int result = uv_write(&info->req, reinterpret_cast<uv_stream_t *>(control->m_tcp), &info->buf, 1,
+ [](uv_write_t *req, int status) {
+ auto info = reinterpret_cast<Info *>(req->data);
+ auto control = info->control;
+ delete info;
+ control->writeCompleted();
+ });
+ if (result != 0) {
+ // The completion callback will not run, so account for it here.
+ delete info;
+ control->writeCompleted();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| request([control = m_control, info](auto loop) { | |
| if (!control->m_tcp) { | |
| delete info; | |
| }); | |
| control->writeCompleted(); | |
| return; | |
| } | |
| info->control = control; | |
| info->buf.base = reinterpret_cast<decltype(info->buf.base)>(const_cast<void *>(info->slice.data())); | |
| uv_write(&info->req, reinterpret_cast<uv_stream_t *>(control->m_tcp), &info->buf, 1, | |
| [](uv_write_t *req, int status) { | |
| auto info = reinterpret_cast<Info *>(req->data); | |
| auto control = info->control; | |
| delete info; | |
| control->writeCompleted(); | |
| }); | |
| request([control = m_control, info](auto loop) { | |
| if (!control->m_tcp) { | |
| delete info; | |
| control->writeCompleted(); | |
| return; | |
| } | |
| info->control = control; | |
| info->buf.base = reinterpret_cast<decltype(info->buf.base)>(const_cast<void *>(info->slice.data())); | |
| int result = uv_write(&info->req, reinterpret_cast<uv_stream_t *>(control->m_tcp), &info->buf, 1, | |
| [](uv_write_t *req, int status) { | |
| auto info = reinterpret_cast<Info *>(req->data); | |
| auto control = info->control; | |
| delete info; | |
| control->writeCompleted(); | |
| }); | |
| if (result != 0) { | |
| delete info; | |
| control->writeCompleted(); | |
| } |
📍 Affects 1 file
src/support/uvfile.cc#L952-L966(this comment)src/support/uvfile.cc#L987-L1002
🤖 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/support/uvfile.cc` around lines 952 - 966, Handle synchronous uv_write
failures in both the raw-pointer and Slice&& overloads of the write operation at
src/support/uvfile.cc lines 952-966 and 987-1002: capture the return value, and
when it is negative, delete the allocated Info and call
control->writeCompleted() before returning. Keep the existing completion
callback behavior for successful submissions.
Comment on lines
+1005
to
1015
| void PCSX::UvFifoListener::failed(int code) { | ||
| m_lastErrorCode.store(code, std::memory_order_release); | ||
| uv_close(reinterpret_cast<uv_handle_t *>(&m_server), [](uv_handle_t *handle) { | ||
| UvFifoListener *listener = reinterpret_cast<UvFifoListener *>(handle->data); | ||
| listener->m_status.store(Status::Failed, std::memory_order_release); | ||
| // Same wake-up an orderly stop() produces, so the consumer's nullptr | ||
| // branch runs and closes its async instead of leaking it. | ||
| listener->m_pending.Enqueue(nullptr); | ||
| uv_async_send(listener->m_async); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
m_status is only settled inside the close callback, so a stop() dequeued in the same batch still double-closes.
request() lambdas are drained in a loop on the worker thread, while the uv_close callback only runs at the end of the loop iteration. If start()'s body calls failed() and a queued stop() is dequeued in that same drain, m_status is still Starting, stop()'s guard passes, and uv_close is invoked on an already-closing handle — the exact !uv__is_closing(handle) abort this change targets. Same window exists for two stop() calls in one batch. Store the terminal status before closing, and/or guard with uv_is_closing.
🛡️ Proposed fix
void PCSX::UvFifoListener::failed(int code) {
m_lastErrorCode.store(code, std::memory_order_release);
+ m_status.store(Status::Failed, std::memory_order_release);
+ if (uv_is_closing(reinterpret_cast<uv_handle_t *>(&m_server))) return;
uv_close(reinterpret_cast<uv_handle_t *>(&m_server), [](uv_handle_t *handle) {
UvFifoListener *listener = reinterpret_cast<UvFifoListener *>(handle->data);
- listener->m_status.store(Status::Failed, std::memory_order_release);
// Same wake-up an orderly stop() produces, so the consumer's nullptr
// branch runs and closes its async instead of leaking it.
listener->m_pending.Enqueue(nullptr);
uv_async_send(listener->m_async);
});
}and correspondingly in stop():
auto status = m_status.load(std::memory_order_acquire);
if ((status == Status::Failed) || (status == Status::Stopped)) return;
+ m_status.store(Status::Stopped, std::memory_order_release);
uv_close(reinterpret_cast<uv_handle_t *>(&m_server), [](uv_handle_t *handle) {
UvFifoListener *listener = reinterpret_cast<UvFifoListener *>(handle->data);
- listener->m_status.store(Status::Stopped, std::memory_order_release);
listener->m_pending.Enqueue(nullptr);
uv_async_send(listener->m_async);
});🤖 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/support/uvfile.cc` around lines 1005 - 1015, Update
UvFifoListener::failed() and stop() so terminal status is established before
invoking uv_close, and ensure each path checks uv_is_closing (or otherwise
atomically prevents repeated closure). Preserve the existing wake-up behavior
while preventing queued failed/stop requests or multiple stop() calls in the
same worker drain from closing m_server more than once.
The Windows build lists its sources explicitly, so the two new files were never compiled there and the link failed on every symbol in them. Add them to the support, gui and test projects, and to the filters so they land in the right folder. asan caught two leak families. uv_connect_t was deleted on the connect failure path and never on success, which the new tests are the first thing to exercise. And the tests themselves ignored uv_loop_close returning EBUSY, which leaks the loop's internals whenever a handle is still open - so close the handles first and pump until the close takes. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The new network tests are the first thing in that project to use libuv directly, and it only referenced gtest, supportpsx, support, tracy and the leak detector, so the Windows link failed on every uv and curl symbol. pcsxrunner already references both for the same reason. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Pulling libuv in leaves the Winsock symbols undefined, since the project declared no AdditionalDependencies at all and inherited only what common.props provides. Use the same list pcsxrunner already carries. ReleaseWithClangCL had no Link section, which is the configuration CI builds, so it gets one. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The GDB server, the web server and the SIO1 pair each hand-rolled the same
bind/listen sequence, each declared its own three-state status enum, and each
dropped every error silently. Nothing read those enums, and both servers carry a
m_gotErrormember that is never written to. So a server that fails to bindleaves its checkbox ticked and says nothing, and unticking it afterwards trips
uv_close's!uv__is_closingassertion.Everything goes through
UvFifoListenernow, and connections areIO<File>.Network::Endpointcarries one status vocabulary and a last error; a concreteendpoint says what to do with a connection and nothing else, which deletes 191
lines from the GDB server and 147 from the web server.
Three transport fixes were needed to get there:
UvFifoListenerreports bind failures instead of closing and returning. Thisis what fixes the abort.
UvFifo::close()flushes pending writes.uv_closecancels queued writes, soclosing right after writing truncated the tail - 64MB written then closed
delivered 2634240 bytes. The web client's
m_closeScheduledbookkeepingexisted to work around this and is now gone.
m_connecting, which was only cleared on success. TheSIO1 reconnect button is gated on
!connecting() && fifoError(), so it couldnever become clickable after the failure it exists to recover from.
UvFifoalso gains an optional readable notification, so the ported serversstay event-driven instead of polling at frame granularity like SIO1 does.
The services move out of the emulation configuration window into a Network
window with a status indicator, a restart button and the error string. The SIO1
client host field loses
ImGuiInputTextFlags_CharsDecimal, which made itimpossible to type a hostname into it.
Unrelated but needed along the way: test objects were built in place by make's
built-in suffix rule, so they had no header dependency tracking and
cleanleftthem behind. Editing a header a test includes relinked the stale object, which
is heap corruption at runtime rather than a build error when the header changes
a class layout. They build under
objs/now.Tested with new gtests in
tests/support/, and against running builds: GDBanswers
?/qSupported/gcorrectly, the web server returns full 1MB and 2MBbodies, SIO1 accepts connections.