Repository navigation
fix(libkernel): wait for the rest of a MSG_WAITALL stream receive in the guest layer - #11896
ionut27001 wants to merge 1 commit into
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The Windows coverage is useful, and the core loop returns partial bytes on timeout/EOF as described. I also built the exact Udp.cpp and guest_socket_wait_all_tests source locally. The binary could not complete under QEMU because this environment's process-memory read helper fails with ENOSYS (process_vm_readv failed with errno 38); I do not count that as a runtime pass. The hosted Windows test passes; Linux and macOS product tests were not run.
[P1] Preserve cancellation and signal handling in the new wait loop. WaitReady calls poll/WSAPoll directly, and on POSIX retries EINTR without consulting the guest I/O cancellation state. When composed with open PR #11872, this is a new blocking phase after a partial receive that bypasses the cancellation-aware ThreadCancel::Native/IoInterrupted path in that PR's WaitForTransfer. A deferred cancellation or caught signal can therefore remain stuck in the post-partial MSG_WAITALL wait. Please coordinate the stack with #11872, route this wait through its cancellation/signal machinery, and add a regression that cancels/interrupts a MSG_WAITALL receive after its first partial bytes.
[P2] Declare the overlapping PR order. The body says this conflicts with #11872 and that whichever lands second will rebase, but that is not an agreed order. CONTRIBUTING.md requires contributors to agree on ordering for conflicting changes and list the dependency. Please coordinate with the #11872 author and record the agreed Depends on order before merge.
The new partial MSG_PEEK | MSG_WAITALL path deliberately throws when fewer bytes are queued, while the measured console repeats queued bytes. Please add a focused assertion for that explicit unsupported path so it remains a deliberate throw, or implement the measured behavior. Linux runtime validation is also still needed because this changes the POSIX receive path.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@ionut27001 I am coordinating #11872 (POSIX read/receive cancellation, head bcf5df1) with this PR under CONTRIBUTING.md. Proposed order: #11872 first, then #11896. Marc identified the post-partial WaitReady integration issue in your review. I propose #11872 first so the new wait loop can use established IoInterrupted/native wake/drain ownership rather than raw poll/WSAPoll retries. Every post-partial wait needs cancellation/signal/close participation, including Windows where CancelIoEx cannot wake an unrelated raw WSAPoll. Partial bytes must remain reportable and HostWrite/descriptor ownership must last through native drain. Please retain per-call flag/timeout contracts and keep console-derived behavior separate unless established for the same entry point. Combined regression checks should cancel, signal and close after a deterministic first partial transfer. I have not run this combination. Would you agree to that order and record #11872 in your Depends on line for the ordering? If your PR needs to go first, please say so; I can handle the later conflict resolution on #11872 instead. This is a proposal awaiting your confirmation, not an existing agreement. AI-assisted coordination. |
|
The proposed order (#11872 first, then this follow-up) is technically coherent because the post-partial wait needs the cancellation-aware transfer machinery. I cannot treat that order as agreed until #11872’s author confirms it; please coordinate with them and record the same |
…the guest layer MSG_WAITALL on a stream socket is no longer passed to the host. recv, recvfrom and recvmsg take the first data with the usual wait, then wait for the rest, also on a nonblocking socket and with MSG_DONTWAIT, and return the partial count on timeout, as a payload process on a PS5 (firmware 12.70) does. A peer close ends the wait with the partial count (FreeBSD 11.4 soreceive_generic; the console was measured only with the close before the receive). MSG_PEEK with MSG_WAITALL on a stream socket throws when the peek returns some but fewer bytes than the request; with nothing queued it behaves as a plain MSG_PEEK.
f2f5410 to
ef5ae8e
Compare
|
@Felipebmaitan agreed: #11872 first, then this one. The body now says The test now also asserts the deliberate throw for |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the measurements and the Windows run. The change itself matches them (cases c, d, f, g in the new test), but it can't land yet:
core/libs/prx/libkernel/Socket/src/Udp.cpp:772WaitReadycallspoll/WSAPolldirectly, andReceive(Udp.cpp:844) loops on it after the first bytes. That wait can't be cancelled, interrupted by a signal or woken byclose, which is what @MarcZX10's open review asks for.- As agreed in the thread, this follows #11872: once it merges, rebase, route the wait for the rest through its cancellation/signal/close handling, and add the regression that cancels, signals and closes during a
MSG_WAITALLreceive after its first partial bytes (partial count still returned).
Everything else (flag handling, MSG_PEEK | MSG_WAITALL throw, debt entry, test registration) looks fine as is.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
I rechecked exact head ef5ae8e against current main 3a2b919; the PR merges cleanly with main, but its declared prerequisite #11872 has since closed as superseded by #12682, and the two current heads conflict when combined.
What I ran
Environment: Linux x86_64, kernel 6.1.0-54-amd64, Python 3.11.2.
git diff --check upstream/main...refs/remotes/review/11896
python3 tools/check_conventions.py --base upstream/main --head refs/remotes/review/11896
git merge-tree --write-tree upstream/main refs/remotes/review/11896
git merge-tree --write-tree refs/remotes/review/12682 refs/remotes/review/11896
gh pr checks 11896 --repo boykopovar/AnyPS5I inspected the current dependency declaration, the closure note on #11872, and the changed files on both PR heads. I did not rerun the socket tests locally.
Results
| Check | Result |
|---|---|
| Merge preview with current main | Clean |
| Diff check | Passed |
| Conventions | Passed |
| Pairwise preview with replacement #12682 | Conflicts in Udp.cpp and docs/dev/debt/libkernel.md |
| Hosted Windows job | Passed |
| Hosted conventions, eligibility, labels, report | Passed |
| Hosted Linux and specialized jobs | Skipped |
| Local socket tests | Not run |
Raw output
Current-main merge preview:
6877f55002b2a534d6c0de5a887f04026664fff6
Diff check:
(no output; exit status 0)
Convention checker:
All conventions checks passed.
Pairwise merge preview:
0459f67d023698e096d420903d78747329c93dce
100644 1131f91a2ae9c3cfa6619268c9bf18e5937331a0 1 core/libs/prx/libkernel/Socket/src/Udp.cpp
100644 dcaa98e609a3488192bec384bf2a7201c6daf121 2 core/libs/prx/libkernel/Socket/src/Udp.cpp
100644 2aa0e1997b2ae964371669bea7d93c7450851b15 3 core/libs/prx/libkernel/Socket/src/Udp.cpp
100644 bb9b37c8e63e109df8ab04027be0541ef7c7a6f2 1 docs/dev/debt/libkernel.md
100644 ebc5431552ce51cad3907cdb24e5195d898524b1 2 docs/dev/debt/libkernel.md
100644 ff35a0d857440c68c74c419f7951ef1d13c1d0e2 3 docs/dev/debt/libkernel.md
Auto-merging core/libs/prx/libkernel/CMakeLists.txt
Auto-merging core/libs/prx/libkernel/Socket/src/Udp.cpp
CONFLICT (content): Merge conflict in core/libs/prx/libkernel/Socket/src/Udp.cpp
Auto-merging docs/dev/debt/libkernel.md
CONFLICT (content): Merge conflict in docs/dev/debt/libkernel.md
Hosted checks:
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40518796924/job/122239970338
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40518796924/job/122240454110
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40518796924/job/122240540630
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40518796924/job/122240660677
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40518796924/job/122241780183
conventions pass 17s https://github.com/boykopovar/AnyPS5/actions/runs/40518795589/job/122239734762
eligibility pass 9s https://github.com/boykopovar/AnyPS5/actions/runs/40518796924/job/122239731863
labels pass 7s https://github.com/boykopovar/AnyPS5/actions/runs/40518795489/job/122239863391
report pass 50s https://github.com/boykopovar/AnyPS5/actions/runs/40518796875/job/122239731403
windows pass 5m1s https://github.com/boykopovar/AnyPS5/actions/runs/40518796924/job/122239851399
Negative control
| Assertion | Change | Result |
|---|---|---|
| The follow-up composes with its prerequisite branch | Compared the current #11896 and #12682 heads using git merge-tree | This integration assertion fails: conflicts remain in Udp.cpp and docs/dev/debt/libkernel.md |
Findings
Blocking
- The checklist still says Depends on: #11872, but #11872 closed at 01:56Z with its author stating that #12682 supersedes it. The current #12682 head is open and is the replacement for the cancellation layer this follow-up said it needed. I also checked the two current heads together: they conflict in Udp.cpp and docs/dev/debt/libkernel.md. Please update the dependency to #12682 and coordinate the follow-up order against that replacement; after it lands, rebase this PR and rerun the focused wait-all tests.
Non-blocking
The PR's merge preview against current main is clean, and its current hosted Windows check passes. Those checks do not resolve the stale dependency or the pairwise conflict with the replacement PR.
Sources
- #11896 Udp.cpp at the reviewed head
- #11896 current dependency declaration
- #11872 closure note identifying #12682 as its replacement
- #12682 replacement head
Not verified
I did not rerun guest_socket_wait_all_tests locally or test Linux/macOS runtime behavior. The PR's Windows check passed, but the hosted Linux and specialized jobs were skipped.
To get this to a good state
Update the dependency line to #12682 and coordinate the rebase after that replacement is ready. Then rerun guest_socket_wait_all_tests on the resulting exact head; I will repeat the merge, diff, conventions, and test checks there.
Re-checked — new head, source link correctedThe closure-note URL in my review was mistyped, and I removed the template-file link because the dependency declaration is in the PR description. Sources |
|
Dependency/order tracker update: the read work formerly in #11872 is now #12682; transport is separately published as #12688; writes formerly in #11969 are now #12700. The old two PRs are closed as superseded with their original branches and review history preserved. The previously requested integration seams/order remain in the tracker: #11872 (comment). Historical confirmations refer to the old read ID; no new author agreement or combined runtime validation is implied. Please confirm the corresponding order for this PR using the replacement IDs. No source changes to this PR are made here. |
Re-checked — dependency order clarifiedThe prior agreement was #11872 first, then #11896; its replacement for the read/cancellation prerequisite is #12682. This confirms that predecessor only and does not establish the combined result with the newer transport/write stack. What I ranEnvironment: Linux 6.1.0-54-amd64 x86_64, GitHub CLI 2.23.0; no compiler or tests were run. gh pr view 11896 --json headRefOid,baseRefName,state
gh pr diff 11896 --name-only
gh pr view 12682 --json headRefOid,baseRefName,state
gh pr diff 12682 --name-only
gh pr view 12688 --json headRefOid,baseRefName,state
gh pr diff 12688 --name-only
gh pr view 12700 --json headRefOid,baseRefName,state
gh pr diff 12700 --name-onlyResults
Unedited changed-file output relevant to the overlapNegative controlNot applicable: this was an ordering check; no code or test assertion was changed or exercised. FindingsBlockingThe current #11896 description still says Non-blockingThe old author confirmation establishes #11872 first only. It does not establish author agreement or runtime validation for the replacement transport/write heads. Sources
Not verifiedI did not run pairwise merge previews or the socket tests, and no combined runtime behavior is verified here. To get this to a good stateUpdate the dependency to #12682. Rebase and resolve the overlapping Udp.cpp changes after the relevant replacement layers settle, then rerun |
|
@MarcZX10 @Felipebmaitan @adriadam10 Confirmed with the new IDs: #12682 lands first. This PR then rebases onto it, waits for the rest of a #12688 and #12700 also change |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Still blocked on the same point, and it can't be fixed from this branch until the prerequisite lands:
core/libs/prx/libkernel/Socket/src/Udp.cpp:772WaitReadycallspoll/WSAPolldirectly andReceive(Udp.cpp:844) loops on it after the first bytes, so that wait can't be cancelled, interrupted by a signal or woken byclose.- Order as agreed: #12682 merges first; then rebase, route the wait for the rest through its cancellation/signal/close handling, and add the regression that cancels, signals and closes during a
MSG_WAITALLreceive after the first partial bytes (partial count still returned). Rebase onto #12688/#12700 as well if they land first.
The rest (flag handling, MSG_PEEK | MSG_WAITALL throw, debt entry, test registration) looks fine as is.
What
MSG_WAITALL(0x40) on a stream socket is no longer passed to the host.recv,recvfromandrecvmsgtake the first data with the existing wait (EAGAINat once on a nonblocking socket or withMSG_DONTWAIT, otherwise up toSO_RCVTIMEO), then wait for readability and receive the rest in a loop, also on a nonblocking socket and withMSG_DONTWAIT, until the request is full, the peer closes, an error occurs or no data arrives forSO_RCVTIMEO(each wait gets the whole timeout; with noSO_RCVTIMEOset the wait for the rest is unbounded, which matches v1 not returning from case c), and return the partial count. Datagram sockets keep ignoring the flag.Before, the host decided. Per the Winsock
recvdocumentation,MSG_WAITALLon a nonblocking socket or withMSG_PEEKfails withWSAEOPNOTSUPP, and a blockingMSG_DONTWAIT | MSG_WAITALLis not bounded bySO_RCVTIMEOafter the first poll. On Linux a nonblocking socket orMSG_DONTWAITreturned the queued bytes at once (tcp_recvmsgbreaks on!timeoonce data is copied), where the console waits up toSO_RCVTIMEO; a blocking receive was not bounded by the guest'sSO_RCVTIMEO, because the guest keeps that timeout in its own socket state and does not set it on the host socket. The old code was run only through the new test on Windows (see Tested), which hit ctest's 20 s limit, most likely at its first, blocking, receive; the nonblocking,MSG_PEEKandMSG_DONTWAITdescriptions above come from documentation only. Only Windows was available as a host.MSG_PEEK | MSG_WAITALLon a stream socket is now a plainMSG_PEEK(the usual wait for the first data,EAGAINwhen nonblocking with nothing queued, 0 at end of stream), and it throws when that peek gets some but fewer bytes than the request (not measured on a console except that case): it was host-dependent before (EOPNOTSUPPon Windows, a wait for the full request on Linux), and the measured result (the queued bytes repeated) is recorded in the debt entry instead of reproduced.recvmsgalready writes the passed flags tomsg_flags, which matches the measured 0x40, so it is unchanged. TheMSG_WAITALLentry indocs/dev/debt/libkernel.mdis cut to what is still open.Measured in a payload process on a PS5 (firmware 12.70), loopback TCP pair, 8-byte requests (probe p85, v2 log):
case a_blocking_split ret=8 errno=0 bytes="12345678" ms=200SHUT_WR:case b_peer_shut_first ret=3 errno=0 bytes="123" ms=0,case b_peer_shut_second ret=0 errno=0 bytes=- ms=0O_NONBLOCK,SO_RCVTIMEO1 s, 3 bytes queued:case c_nonblock_3queued ret=3 errno=0 bytes="123" ms=1000; nothing queued:case c_nonblock_empty ret=-1 errno=35 bytes=- ms=0. v1, which per the v2 probe header set noSO_RCVTIMEO, logged nothing afterc_setnonblockand has no# end probeline.case g_recvfrom_nonblock_3queued ret=3 errno=0 bytes="abc" ms=1001,case g_recvmsg_nonblock_3queued ret=3 errno=0 bytes="xyz" ms=1000,case g_recvmsg_flags value=0x40MSG_DONTWAIT | MSG_WAITALL,SO_RCVTIMEO2 s, 3 bytes queued:case d_dontwait_waitall_3queued ret=3 errno=0 bytes="123" ms=2000MSG_PEEK | MSG_WAITALL, 3 bytes queued:case e_peek_waitall_3queued ret=8 errno=0 bytes="12312312" ms=0, thencase e_plain_after_peek ret=3 errno=0 bytes="123" ms=0SO_RCVTIMEO300 ms, 3 bytes queued:case f_timeout_3queued ret=3 errno=0 bytes="123" ms=300; nothing queued:case f_timeout_empty ret=-1 errno=35 bytes=- ms=300The new test
guest_socket_wait_all(targetguest_socket_wait_all_tests) covers c, d, f, g, themsg_flagsread-back, the empty nonblockingEAGAIN,MSG_PEEK | MSG_WAITALLwith nothing queued (EAGAIN) and the deliberate throw forMSG_PEEK | MSG_WAITALLwith 3 of 8 bytes queued (the bytes stay queued) on a loopback TCP pair with a 200 msSO_RCVTIMEO; a and the first receive of b stay covered byguest_tcp. The three receives now share oneWSAEMSGSIZEmapping on Windows, which also accepts sequenced-packet sockets forrecvandrecvfromasrecvmsgalready did; Windows has noSOCK_SEQPACKETsocket (socketpairthrows), so this cannot be observed.Tested
Windows 11, WinLibs GCC 15.2.0 (Release, Ninja), commit ef5ae8e on main 75f3702:
ctest -R ^guest_socket_wait_all$passed twice (about 1.5 s each).ctest -R "guest_tcp|guest_udp|socket|guest_unix|guest_net": 43 of 43 passed.Udp.cpprestored from main and rebuilt,guest_socket_wait_alltimes out (ctest's 20 s limit), most likely in the first blocking receive, where the hostMSG_WAITALLreceive is not bounded by the guest'sSO_RCVTIMEO.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)Udp.cpp; this rebases onto whichever of them land first