Expose normalized transport-error detail (ErrorString/ErrorSymbol/ErrorCode) — lands #31 + review fixes#33
Merged
Conversation
Transport failures (DNS, connection refused, TLS, missing local files) previously completed with StatusCode 0 and no further information: the Apple backend discarded the NSError (an existing TODO), and the curl backend caught and dropped its own failure exception. Consumers like JsRuntimeHost's fetch/XMLHttpRequest polyfills could only report 'network request failed', which makes field crash reports undiagnosable. New accessors, empty/zero when the request did not fail at the transport layer, normalized for log-pipeline (Splunk etc.) substring filtering: ErrorString() -> "<domain>:<symbol>(<code>): <detail>" ErrorSymbol() -> e.g. CURLE_COULDNT_RESOLVE_HOST, NSURLErrorTimedOut ErrorCode() -> raw numeric platform code - Apple: capture NSError domain/code/description (resolves the TODO), walking the underlying-error chain for distinct-code levels; missing app:/// bundle resources report urllib:AppResourceNotFound(0). - curl: CURLOPT_ERRORBUFFER detail (host/port/path specifics) with curl_easy_strerror fallback and stable CURLE_* symbols. - Purely additive: SendAsync still completes normally with status 0, so existing consumers are unaffected until they opt in. - Windows/Android backends still report empty/zero (follow-up). Adds offline-deterministic gtest coverage (7 cases: success, missing file, connection refused, NXDOMAIN via RFC 6761 .invalid, grammar shape, reuse-clears-error, missing app resource) and a CI workflow running them on ubuntu-latest (curl path) and macos-latest (NSURLSession path).
…imeout - RefusingPort: race-free held-bind refusal on Linux; on Darwin a SYN to a bound-but-not-listening port is dropped (times out, -1001) rather than refused, so close after reserving the port number there. optional<> + ASSERT at call sites instead of EXPECT inside the helper. - TempFile: pid-suffixed unique names, removed on destruction. - SendAndWait: 30s wait, Abort() on timeout (currently only effective on the Windows backend; the offline-deterministic scenarios are the real bound).
Replace the single-file matrix workflow with the family pattern used by both sibling repos: a ci.yml orchestrator calling reusable per-platform workflow_call files (build-linux.yml, build-macos.yml), checkout@v5, timeout-minutes, Ninja + explicit gcc/clang jobs on Linux, pinned Xcode selection with -G Xcode on macOS, Build/<platform> dirs, and running the test binary directly. Verified the macOS steps locally (Xcode generator, RelWithDebInfo, 7/7 tests).
build-win32.yml mirrors JsRuntimeHost's: windows-2022 (VS2022 generator pin), -A platform input, RelWithDebInfo with /m, crash-dump registry staging and artifact upload on failure. Test fixtures now compile on MSVC: Winsock variant of RefusingPort (WSAStartup, SOCKET/closesocket), _getpid, and file:///C:/... URL formation. The five assertions that depend on transport-error detail GTEST_SKIP on Windows with an explicit message, since the Windows backend does not populate the new accessors yet -- the gap stays visible in test output while the suite (and the local-file success case, which runs for real) stays green.
The GitHub Actions setup now lives in its own PR so the infrastructure can be reviewed independently; it depends on this PR's UrlLibTests target and lands after it.
- Document that a transport failure is signaled by a non-empty
ErrorString()/ErrorSymbol(), not ErrorCode() != 0; AppResourceNotFound
legitimately reports code 0 (header + README).
- Apple: add ToStdString() so a nil NSString (localizedDescription/domain)
cannot construct std::string{nullptr} (undefined behavior).
- Unix: rename the synthesized getinfo-failure token from CURLE_GETINFO_FAILED
to GetInfoFailed so it does not masquerade as a real CURLcode.
- Tests: loosen the curl DNS assertion to a "curl:" transport error with a
non-zero code (tolerates proxy/hijacking resolvers; exact symbols stay
covered by the refused/missing-file cases) and assert the AppResourceNotFound
code-0 contract explicitly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds normalized transport-layer error detail reporting to UrlRequest via three new additive accessors (ErrorString(), ErrorSymbol(), ErrorCode()) and wires the feature into the Apple (NSURLSession) and Unix (libcurl) backends, along with offline-deterministic gtest coverage and documentation updates explaining the “code can be 0” contract.
Changes:
- Add shared error-state plumbing in
ImplBaseplus publicUrlRequestaccessors for normalized transport-error detail. - Populate error detail on Apple (NSError mapping + underlying-error chain) and Unix (libcurl perform failure + error buffer).
- Introduce
UrlLibTests(googletest) with deterministic transport-failure scenarios and document usage/semantics inREADME.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UrlRequestErrorReporting.cpp | Adds offline-deterministic gtests covering transport-failure detail and reuse semantics. |
| Tests/CMakeLists.txt | Adds UrlLibTests target and pulls in googletest via FetchContent. |
| Source/UrlRequest_Unix.cpp | Populates curl error detail (symbol/code/message) and uses CURLOPT_ERRORBUFFER. |
| Source/UrlRequest_Apple.mm | Populates Apple transport error detail (stable symbol mapping + underlying chain) and adds nil-safe string conversion. |
| Source/UrlRequest_Base.h | Adds shared storage/accessors for error fields plus SetError() and reset behavior. |
| Source/UrlRequest_Shared.h | Exposes new accessors on UrlRequest by forwarding to the impl. |
| Include/UrlLib/UrlLib.h | Adds public API declarations + documentation comments for new error accessors. |
| README.md | Documents transport error reporting, including the “non-empty string is the failure signal” contract. |
| CMakeLists.txt | Adds URLLIB_TESTS option and hooks in Tests/ when building top-level (desktop). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
3
to
7
| #include <curl/curl.h> | ||
| #include <unistd.h> | ||
| #include <array> | ||
| #include <cstring> | ||
| #include <filesystem> |
Comment on lines
+145
to
+147
| // surface it. NSURLErrorDomain codes get stable symbols; other domains pass | ||
| // through verbatim. One level of NSUnderlyingErrorKey is appended because | ||
| // that is where CFNetwork/POSIX specifics (e.g. "Connection refused") live. |
Comment on lines
+91
to
+108
| [[nodiscard]] bool SendAndWait(UrlLib::UrlRequest& request) | ||
| { | ||
| auto done = std::make_shared<std::promise<void>>(); | ||
| auto future = done->get_future(); | ||
|
|
||
| request.SendAsync().then(arcana::inline_scheduler, arcana::cancellation::none(), | ||
| [done](const arcana::expected<void, std::exception_ptr>&) { | ||
| done->set_value(); | ||
| }); | ||
|
|
||
| if (future.wait_for(std::chrono::seconds{30}) != std::future_status::ready) | ||
| { | ||
| request.Abort(); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
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
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.
Lands the transport-error reporting work from #31 (authored by @matthargett) together with maintainer review fixes applied on top.
This is pushed as a base-repo branch because #31's head fork is org-owned, so GitHub blocks maintainer-edit pushes onto its branch despite
maintainerCanModify == true. #31 will be closed manually once this merges; all of @matthargett's original commits are preserved here.What #31 adds (unchanged)
UrlRequest::ErrorString()/ErrorSymbol()/ErrorCode()— normalized, grep-friendly transport-error detail ("<domain>:<symbol>(<code>): <detail>") for the Apple (NSURLSession) and Linux (libcurl) backends. Purely additive:SendAsync()still completes with status 0 on transport failure, so existing consumers are unaffected. Offline-deterministic gtest coverage included.Maintainer review fixes (commit
51bbf33)app:///resource legitimately reportsAppResourceNotFoundwith code0, so the docs now state that a transport failure is signaled by a non-emptyErrorString()/ErrorSymbol()— not byErrorCode() != 0. A test asserts the code-0 contract explicitly.ToStdString(NSString*)so a nillocalizedDescription/domaincan't constructstd::string{nullptr}(UB) when building the error detail or walking the underlying-error chain.CURLE_GETINFO_FAILED→GetInfoFailedso it no longer masquerades as a realCURLcode..invalidassertion now requires acurl:transport error with a non-zero code instead of pinningCURLE_COULDNT_RESOLVE_HOST(6), tolerating proxy / DNS-hijacking resolvers; exact symbols stay covered by the connection-refused and missing-file cases.Validation
Built and ran
UrlLibTestson Win32 (MSVC): the local-file success case passes; the transport-detail casesGTEST_SKIP(Windows backend doesn't populate detail yet — unchanged from #31). Apple/Linux backends keep #31's CI coverage.Follow-up
CI workflows land separately in #32 (the
.github/churn in this branch's history nets to zero in the tree, matching #31's file set).Co-authored-by: Matt Hargett plaztiksyke@gmail.com