Repository navigation
Add the transport seam and the libcurl default (T10) - #7
kirre-bylund wants to merge 3 commits into
Conversation
IHttpTransport with the pollable contract: Start returns a handle without blocking, Poll reports completion without waiting, Cancel releases. That shape is the whole point of D3 - a blocking Perform called from Update() would stall the host's frame for the length of the transfer. HeaderMap is public because an adopter implementing the interface has to build one. Insertion-ordered and case-insensitive on lookup, not a std::map: header order and case matter to some servers and proxies, and a map would silently reorder them and lose the original casing. Set replaces in place so overwriting Content-Type does not move it to the end. FakeHttpTransport is public rather than test-only, because an adopter writing their own tests needs the same thing. Transfers complete only when the test says so, which is what makes the pollable contract assertable without sleeping. CurlHttpTransport is curl_multi, which is already pollable - Start adds an easy handle, Poll drives the multi handle once, Cancel removes it. The one thing that needs care is that curl's completion notification is separate from the per handle result, so a transfer is only done once both have happened. libcurl is optional, not required. A console adopter injects its own transport and must not be forced to build curl, and a studio that vendors dependencies itself may already have one. LOOTLOCKER_WITH_CURL defaults ON but a missing curl is a status message rather than an error; the header guards on the same macro so an adopter without curl gets a clean build and no dangling declaration. CI now installs libcurl on all three targets, including the module-boundary job. Without that the curl path would be dead code that only a downstream adopter discovers is broken. Verified: 12 test functions, all 9 suites pass, zero warnings. The curl TU was compiled against a stub header to prove it is syntactically sound, since curl is not installed on this machine - that is weaker than a real build, and CI is what closes the gap. Breaking the pollable contract fails 4 assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Approval is blocked by active CMake integration, public-interface, lifetime, global cleanup, and non-blocking DNS issues that prevent reliable and safe use of the curl transport.
5 open issues
What changed in this PR
Adds the T10 pollable HTTP transport seam, including ordered headers, a scriptable fake transport, and an optional libcurl-backed implementation.
Changes:
- Introduces public
IHttpTransport,HeaderMap,FakeHttpTransport, and conditionalCurlHttpTransportAPIs. - Implements header handling and
curl_multitransfer lifecycle management. - Adds transport tests, CMake integration, optional libcurl detection, CI configuration, and build-directory ignores.
| File | Description |
|---|---|
| tests/test_transport.cpp | Adds tests for header behavior and fake transport polling, completion, failure, cancellation, and binary bodies. |
| tests/CMakeLists.txt | Registers the transport test executable with the test suite. |
| src/core/header_map.cpp | Implements ordered, case-insensitive header lookup, replacement, and erasure. |
| src/core/curl_http_transport.cpp | Implements the libcurl curl_multi transport and asynchronous transfer lifecycle. |
| include/lootlocker/http_transport.h | Defines the public pollable HTTP transport interface and request/response types. |
| include/lootlocker/header_map.h | Defines the public ordered, case-insensitive header collection. |
| include/lootlocker/fake_http_transport.h | Provides a public scriptable fake transport for tests and adopters. |
| include/lootlocker/curl_http_transport.h | Declares the conditional libcurl transport implementation. |
| CMakeLists.txt | Adds transport sources, optional libcurl discovery, and curl target integration. |
| .gitignore | Ignores generated CMake build directories. |
| .github/workflows/build.yml | Updates CI configuration for libcurl setup and transport-related build coverage. |
Review effort: Lite
| if(CURL_FOUND) | ||
| target_sources(lootlocker_core PRIVATE src/core/curl_http_transport.cpp) | ||
| target_link_libraries(lootlocker_core PRIVATE CURL::libcurl) | ||
| target_compile_definitions(lootlocker_core PRIVATE LOOTLOCKER_HAVE_CURL) |
There was a problem hiding this comment.
Fixed. LOOTLOCKER_HAVE_CURL is now PUBLIC. You were right that the guard made the class undeclared for an adopter even though it existed in the binary - the documented default transport could not be constructed outside the library at all. I verified the propagation with a standalone CMake project rather than assuming, since the local machine has no curl and the block never runs here.
| curl_multi_cleanup(static_cast<CURLM*>(m_multi)); | ||
| m_multi = nullptr; | ||
| } | ||
| curl_global_cleanup(); |
There was a problem hiding this comment.
Fixed. curl_global_init/curl_global_cleanup are now refcounted at file scope, torn down only when the last transport is destroyed. You were right that this is a use-after-free that only appears when two clients exist - which is exactly the case a single-transport test would never catch. A plain int rather than an atomic, because the SDK is single-threaded by contract (D5) and transports are constructed and destroyed on the Update thread.
| curl_easy_setopt(transfer->easy, CURLOPT_POSTFIELDS, request.body.c_str()); | ||
| curl_easy_setopt(transfer->easy, CURLOPT_POSTFIELDSIZE, | ||
| static_cast<long>(request.body.size())); |
There was a problem hiding this comment.
Fixed, and this is the one I am most glad you caught. CURLOPT_POSTFIELDS borrows the pointer, and it pointed into the caller's Request, which the caller may destroy the moment Start returns. The transfer now owns a copy for its lifetime. I used an owned std::string rather than CURLOPT_COPYPOSTFIELDS because the latter copies again internally and gives no way to free the copy deterministically. This would have corrupted request bodies intermittently and been very hard to diagnose from a bug report.
| int running = 0; | ||
| // A zero timeout is what makes this non-blocking: curl does whatever work is ready and | ||
| // returns immediately rather than waiting for the socket. | ||
| curl_multi_perform(multi, &running); |
There was a problem hiding this comment.
You are right, and I have narrowed the contract rather than pretending. curl's default resolver is synchronous, so the first Poll for an uncached hostname can block for the length of the lookup - which breaks the non-blocking promise the header makes.
I did not add an async resolver: that needs either a curl built with c-ares or a thread, and neither is available here. So the interface now documents the exception explicitly - the transfer is pollable, the name resolution is not, and a game that cannot tolerate that should resolve at startup or inject a transport with an async resolver. I also set CURLOPT_DNS_CACHE_TIMEOUT so repeat lookups do not pay it again.
Flagging it as a known limitation rather than a fix, because it is one. If you would rather the default transport refuse to start without an async resolver, say so and I will make it a configure-time requirement instead.
|
|
||
| # The curl transport is only compiled when curl was found. The header guards on the same | ||
| # macro, so an adopter without curl gets a clean build and no dangling declaration. | ||
| if(CURL_FOUND) |
There was a problem hiding this comment.
Fixed. The block now gates on LOOTLOCKER_WITH_CURL AND CURL_FOUND. You were right that CURL_FOUND can already be true in a parent project or a reused configure, so the documented opt-out did nothing.
windows-latest does not ship curl development files, only the CLI, so the default transport was silently absent on MSVC - the one target where the curl path was never compiled. vcpkg is the supported way to get headers and a lib on the runner. Found by reading the CI log rather than trusting the green check: all six jobs passed while one of them was building a core without the transport.
LOOTLOCKER_HAVE_CURL was PRIVATE, so a consumer including the public curl_http_transport.h saw the guard as false and CurlHttpTransport was undeclared - the documented default transport could not be constructed outside the library. It is PUBLIC now. Verified with a standalone CMake project that a PUBLIC define does reach a consumer TU. The curl target block gated on CURL_FOUND alone, ignoring LOOTLOCKER_WITH_CURL. CURL_FOUND can already be true in a parent project or a reused configure, so the documented opt-out did nothing. Both conditions now. curl_global_init and curl_global_cleanup were per instance. libcurl's global state is process-wide, so destroying one transport tore it down under any other one still holding handles - a use-after-free that only appears when two clients exist. Now refcounted, torn down when the last transport goes. CURLOPT_POSTFIELDS borrows the pointer, and it pointed into the caller's Request, which the caller may destroy the moment Start returns. The transfer now owns a copy for its lifetime. This is the one that would have corrupted request bodies intermittently and been very hard to diagnose. The DNS claim was wrong. curl's default resolver is synchronous, so the first Poll for an uncached hostname can block for the length of the lookup, which breaks the non-blocking promise the header makes. Rather than pretend, the contract is narrowed and documented in the interface: the transfer is pollable, the name resolution is not, and a game that cannot tolerate that should resolve at startup or inject a transport with an async resolver. A DNS cache timeout is set so repeat lookups do not pay it again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implements T10: the transport seam and the libcurl default.
The contract
IHttpTransportis pollable, not blocking:Startreturns a handle without blocking,Pollreports completion without waiting,Cancelreleases. That shape is the whole point of D3 — a blockingPerformcalled fromUpdate()would stall the host's frame for the length of the transfer.Three pieces
HeaderMapis public because an adopter implementing the interface has to build one. Insertion-ordered and case-insensitive on lookup, deliberately not astd::map: header order and case matter to some servers and proxies, and a map would silently reorder them and lose the original casing.Setreplaces in place, so overwritingContent-Typedoesn't move it to the end.FakeHttpTransportis public rather than test-only, because an adopter writing their own tests needs the same thing. Transfers complete only when the test says so, which is what makes the pollable contract assertable without sleeping.CurlHttpTransportiscurl_multi, which is already pollable —Startadds an easy handle,Polldrives the multi handle once,Cancelremoves it. The one thing needing care is that curl's completion notification is separate from the per-handle result, so a transfer is only done once both have happened.libcurl is optional, not required
A console adopter injects its own transport and must not be forced to build curl; a studio that vendors dependencies itself may already have one.
LOOTLOCKER_WITH_CURLdefaults ON, but a missing curl is a status message rather than an error — the header guards on the same macro, so an adopter without curl gets a clean build and no dangling declaration.CI now installs libcurl on all three targets, including the module-boundary job. Without that the curl path would be dead code that only a downstream adopter discovers is broken.
Verification, and one honest gap
-DLOOTLOCKER_MODULE_ONLYstill configures forcoreandauth;broadcasts.The gap: libcurl is not installed on this machine, so the curl TU was compiled against a stub header to prove it is syntactically sound. That is weaker than a real build — it proves the code parses and type-checks against curl's API shape, not that it links or works. CI is what closes that gap, and it is the first thing to check on this PR.
Spec
lootlocker/index#1905 records the transport decisions.