Repository navigation
Conversation
|
The two failure paths are fixed the right way, verified against the diff: Two nits, neither blocking:
No conventions problems: no comments added, the debt entry records exactly what remains unverified (console OOM error values and failed-call outputs), and the inline |
There was a problem hiding this comment.
The allocation-failure fixes match the ownership invariants: init rolls back the inserted context, epoll output is published only after registry insertion, and termination swaps the existing map instead of allocating a teardown vector. The one-shot allocator avoids recursive fault injection during unwinding. The author reports 7/7 regressions on the fix versus 1/7 on base, plus 0/8 output changes after failed epoll creation, 11/11 selected runtime tests and a full Windows build. An independent review agrees with the key failure paths. I ran exact-head git diff --check and current-main conventions; both pass. Full hosted product jobs are skipped on this head, so the Windows results are author-reported; I did not rerun them locally. Minor test coverage note: termination explores budgets 0–3 while init/create explore 0–7, and CMake has duplicated Windows PATH setup; neither changes the core disposition. The change is generic with Unblocks: none. Coordinate merge order with #8357 as stated in the description.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
MarcZX10
left a comment
There was a problem hiding this comment.
The allocation-failure fixes and regression results remain convincing. I independently ran exact-head whitespace/conventions checks, test registration (171 scripts), and Linux syntax checks for the changed HTTP implementation, options unit, and allocation test; all passed. The author reports 7/7 allocation regressions, 0/8 failed-create output mutations, 11/11 selected runtime tests, and a full Windows build. Product jobs are path-skipped in hosted CI, so those runtime results remain author-reported here.
I am withdrawing my earlier #5239 intake hold as a reason to defer this PR. The remaining repository requirement is a current-main base: this targets d6eafa5, while main is 15 commits ahead at 8b3a6ba. The intervening commits do not touch the HTTP files changed here, so the update should be low risk; please rebase and refresh conventions/overlap checks before merge. The ordering with #8357 is already agreed by both authors: this fix first, then #8357 rebases and carries rollback through its pool registry.
|
I am working on a separate libSceHttp request-transport PR for PPSA90210. It needs to register per-context net/SSL ids in |
|
@DoubleAAGuy I prefer #9630 first, then your transport PR with For the combined initialization/termination hunk:
Please also account for #8357: its pool-size registry changes the same exports and already declares #9630 as its prerequisite. I would compare the transport and pool-stat hunks before choosing their relative order, preserving both registries. I have recorded the proposed transport sequencing in this PR's description. Please link your branch or PR when available so I can compare the actual changes and help settle the remaining order with #8357. AI-assisted: yes. |
|
Agreed. I have the transport context registration inside the |
|
The transport work is now in draft #9834. It declares |
|
I’ve noticed that my earlier request to hold this PR based on #5239 was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. Please disregard that request. |
|
Update: #8357 merged before #9630. I rebased #9834 onto current main and made its init rollback cover the merged pool-size registry together with HTTP options and the new transport-context registry. The fault-injection test verifies failed init leaves neither pool stats nor transport templates for the failed context, including with an existing context. All five focused HTTP tests pass under x86-64 QEMU; native x86-64 validation remains outstanding. |
|
Hi @Ondra97, I'm the author of #10535 (libSceHttp cookie store), which overlaps with this PR in libSceHttp/CMakeLists.txt: both append a BUILD_TESTING block at the end of the file. Would you be OK with #9630 merging first? I'd then rebase #10535, keep both test blocks, and add If you'd rather #10535 go first, I'll post that one-line change here for your rebase. Either way I'll declare the order with Depends on in #10535. |
|
@noamroger Agreed: #9630 first, then #10535 with I checked the CMake/source dependencies at your head Please rerun the allocation-failure cases and cookie tests on the combined sources after the rebase. This was a static comparison on my side; I have not built the combined patch. |
|
Thanks @Ondra97 ! I merged #9630 into 6528fc9 locally, kept both test blocks and added src/Cookies.cpp src/Uri.cpp to guest_http_state_allocation_tests. All seven allocation cases (init, init-live, create, term 0-3) pass, as do the cookie, options and response-header tests. Without the two sources the target fails to link (ClearHttpCookieJar/RemoveHttpCookieJar), as expected. I'll push that resolution to #10535 once #9630 lands. |
|
Independent verification of the outstanding ask (current-main refresh + refreshed focused rerun) as a combined both-stacks run — I ran the integration the comments above requested coordination on, on a local merge of both this stack and #9598's (their diffs are disjoint: Heads
Results (same machine, same build, parent vs change)Arch Linux (CachyOS, kernel 7.2.10-1-cachyos), GCC, Ninja,
Base: 7 of 8 fail, 3 consecutive runs identical. Change: 8/8 pass, 3 consecutive runs each in Release and ASan (12 runs total), each run reports 100% and no ASan report. In the base runs the failures are the allocation-pressure lifecycles this PR's report covers (e.g. Conventions: Not run: Windows, macOS, a title, console. The combined-stack caveat from the discussion above still stands: this establishes both allocation-failure fixes work together on local Linux/ASan bookkeeping behavior, not console child-handle lifetimes or cancellation of already-posted events (recorded in this PR's debt). |
|
Native Windows result for this head, Setup: Windows 11 Pro (26300), Ryzen 5 7600X, WinLibs GCC 15.2.0 (UCRT), Release, full build. Results:
Note for #9834, which lists this PR as a dependency:
AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
The exact head f66f955da716869c32b74a0d822e4ddea71ca43e preserves the intended rollback invariants: failed initialization removes the unpublished context, epoll output is not published before registry insertion succeeds, and termination swaps the existing map instead of allocating during teardown. The author reports 7/7 lifecycle regressions on the fix versus 1/7 on the base, plus 11/11 selected HTTP tests and a full Windows build. An independent Windows run of the same head merged onto current main reports all 10 libSceHttp tests passing. The earlier Linux syntax, conventions, overlap, and test-registration checks also passed; hosted product jobs remain path-skipped, and I did not rerun a local build.
The order with #8357 and #10535 is documented, and the dependent #9834 author has been told its allocation test target needs source/link changes when stacked. No blocking correctness issue remains in this PR. Approved.
jotitadev
left a comment
There was a problem hiding this comment.
Merging this head into current main breaks public_headers.
Ubuntu 26.04, i7-8700, GCC 15.2, Release, BUILD_TESTING=ON. Head f66f955da7 merged (--no-commit) onto main f91137142e. The merge is clean, and the 7 guest_http_state_* tests pass 3/3.
public_headers fails. The PR's base b7d1d47a5f predates #10706, which requires each export to be declared once, in its library's public header. The extern "C" block in HttpStateAllocation.cpp declares 7 exports again that prx/libSceHttp/include/Http.hpp already declares:
sceHttpCreateEpoll: expected exactly one header declaration; found 2: core/libs/prx/libSceHttp/include/Http.hpp:48, core/libs/tests/HttpStateAllocation.cpp:17
The same happens for sceHttpInit_nid_postfix, sceHttpTerm_nid_postfix, sceHttpAuthCacheFlush, sceHttpGetNonblock, sceHttpDestroyEpoll and sceHttpWaitRequest. The build CI doesn't run on PRs, so it didn't show up here.
Including the header instead of the block fixes it:
#include "prx/libc/include/general/VabiMacros.hpp"
+#include "prx/libSceHttp/include/Http.hpp"
#include "prx/libSceHttp/src/HttpErrors.hpp"
...
-extern "C" {
-int APS5_VABI sceHttpInit_nid_postfix(int, int, std::uint64_t);
-...
-int APS5_VABI sceHttpWaitRequest(HttpEpollHandle, HttpNBEvent*, int, int);
-}
-With that change, on the same merge:
- The audit reports 0 errors;
public_headersand the 64public_headers_compile_*tests pass (65/65). - Full build OK. The 7
guest_http_state_*tests pass 3/3, and all HTTP/HTTP2 tests pass (19/19). - Control: with
Export.cppfrommain, 6 of 7 fail 3/3 (init,init_live,create_epoll,term_0toterm_2), the same as your red control.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
jotitadev
left a comment
There was a problem hiding this comment.
Re-checked cff855f726 on Linux: public_headers passes now.
Ubuntu 26.04, i7-8700, GCC 15.2, Release, BUILD_TESTING=ON. Head merged (--no-commit) onto current main 16345cad6a; clean merge, full build OK.
public_headers, the 64public_headers_compile_*tests and the 7guest_http_state_*tests: 72/72, 3 of 3 runs.- All HTTP tests: 20/20.
- Red control: with
Export.cppfrommain, 6 of 7guest_http_state_*still fail 3/3 (init,init_live,create_epoll,term_0toterm_2), same as before.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
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.
|
What
Allocation failure can leave HTTP context or epoll ownership partly published. Initialization now treats main's pool-size registry, registered net/SSL IDs, context set and option initialization as one transaction: if any insertion fails, the unpublished context's entries are removed and existing contexts remain usable. Epoll handles are written only after registry insertion succeeds. Last-context termination detaches the existing epoll map with
swap, avoiding an allocating teardown list after context removal.The public-header review is addressed by including
prx/libSceHttp/include/Http.hppinHttpStateAllocation.cppand removing all seven local export redeclarations. The one-shot allocation injector remains disarmed before throwing, so exception unwinding cannot reinject the same synthetic fault.The current-main refresh also closes the rollback gap introduced by the two newer registries. The live-context regression queries both public registry APIs for the unpublished ID and checks that the existing context's pool stats and registered IDs remain valid. The fixture derives the attempted ID from this implementation's monotonic initializer; it makes no PS5 handle-layout claim. PS5 OOM errors and failed-call outputs remain unmeasured in debt, and creation may still throw
std::bad_alloc.The agreed order remains #9630 before cookie-store #10535 and draft transport #9834. #11134 depends on #9834 and carries its transport integration. None is a prerequisite for this standalone bookkeeping fix. Future composition must retain rollback across all registries and preserve both test blocks. The cookie/transport source and link additions discussed in the thread remain required for their combined allocation target; no new combined-stack run is claimed here.
Tested
Revalidated on 2026-10-30: base
556547da71c5e25b742f17b1ed9b27253b6e9d4d, submitted headcff855f72618fbee7bde0b866609854017ca5f9b. Main was merged into the existing branch, retaining the original allocator and epoll patches.Windows 11 Pro 10.0.26300, AMD Ryzen 9 7900X, 31.7 GiB RAM, NVIDIA GeForce RTX 4070 Ti SUPER (unused by these CPU tests); WinLibs MinGW-w64 GCC 15.2.0 r7 UCRT; Release,
BUILD_TESTING=ON,ANYPS5_ENABLE_SPIRV_TOOLS=ON, pinned bundled dependencies.relinker/libs, patched-library checker and all selected HTTP/HTTP2 test executables built successfully on the submitted sources.failed init retained its pool-size registrationon the header-fixed tree with the old rollback. Moving all three main-side registrations into the transaction and erasing them on failure makes the same test pass. The options registry insertion has the container's strong failure guarantee. The full suite and repeated allocation cases were then rerun on the final sources.Conventions, strict main synchronization, whitespace and registration checks pass. The overlap report identifies #9834/#11134's transport transaction and #10535's adjacent CMake registration; the shared hunks were inspected and the existing dependency direction is retained. The transport variants already carry their own additional registry rollback, which must be combined with this standalone fix. No peer implementation was copied into this branch and no transport/cookie combined runtime validation is claimed.
No title, console, complete CTest suite or native Linux run of this refreshed head is claimed. The reviewer's Linux header/build/red-control results concern the stated earlier head and local header patch. Earlier author validation below remains historical.
Earlier submitted-revision validation (historical)
Windows 11 Pro 10.0.26300; AMD Ryzen 9 7900X, 31.66 GiB RAM, NVIDIA GeForce RTX 4070 Ti SUPER. WinLibs GCC 15.2.0, Ninja Release,
BUILD_TESTING=ON,ANYPS5_RELINKER_ONLY=OFF, bundled dependencies.Current main base:
b7d1d47a5f9f32c9c388ad8d47a0055a90ae6e33. Tested head:f66f955da716869c32b74a0d822e4ddea71ca43e. The regression executable compiles productionExport.cppandOptions.cppdirectly so its replacement C++ allocator reaches the real bookkeeping code. The red control and the Before values below were measured on the earlier based6eafa54d0464258574fd2f71889754d05a5bc93; I did not rerun that control on the new base. The HTTP implementation and regression source were unchanged by the main merges. The original HTTP tests separately exercise the shared library.With the one-shot injector, the unchanged base failed 6/7 new lifecycle tests in 0.25 s: initialization left partial state at budget 2 without an existing context and at budget 1 with one; epoll insertion changed the output in two failed calls; termination lost the context at budgets 0-2. The fix passed all 7 new tests and 28 budget scenarios: 8 for each initialization mode, 8 for epoll creation and 4 for termination. The base initialization probes stop at their first invalid state, so they do not complete all 28 scenarios. Successful epolls support polling/deletion, existing contexts survive failed creation, failed outputs remain untouched, and a fresh context cannot use epolls from a completed termination. No title or console was run.
The full all-target Windows build passed on the final head with
BUILD_TESTING=ON, bundled dependencies and the complete default AGC execution-test set. #8310 is merged in the base; no source overlay was used. An initial sandboxed build stopped in the libusb version generator because Git Bash/MSYS could not create a named object (0xC0000022); the same incremental build completed outside that sandbox.Final selected runtime tests: 12/12 passed, 0 skipped; 367 patched libraries loaded, 0 failed. Both changed translation units compile; conventions, registration (1,176 test executables, none built and unregistered), main-sync and whitespace checks pass. Registration is an executable count, not an executed-test count.
Commands, with the supported WinLibs toolchain on
PATHand the full Release build configured in a sibling../builddirectory:All commands above used the final submitted sources. The earlier red control temporarily compiled the regression target with unchanged earlier-base
Export.cpp, then restored the source before the final build.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)