TEMP DIAG (do not merge): capture Win32-Release FuncTests hang stacks - #1522
Draft
bmehta001 wants to merge 69 commits into
Draft
TEMP DIAG (do not merge): capture Win32-Release FuncTests hang stacks#1522bmehta001 wants to merge 69 commits into
bmehta001 wants to merge 69 commits into
Conversation
Under -Werror on Linux/macOS, the modules-repo CI (build-posix-latest-exp)
has been failing for ~2 weeks with:
config-default.h:36: error: 'HAVE_MAT_LIVEEVENTINSPECTOR' macro redefined
[-Werror,-Wmacro-redefined]
config-default.h:37: error: 'HAVE_MAT_PRIVACYGUARD' macro redefined
tests/functests/CMakeLists.txt and tests/unittests/CMakeLists.txt add
-DHAVE_MAT_LIVEEVENTINSPECTOR / -DHAVE_MAT_PRIVACYGUARD on the command
line when BUILD_LIVEEVENTINSPECTOR / BUILD_PRIVACYGUARD (default YES) and
the respective module dir exists. The three config-default headers then
redefined them unconditionally, which is fatal under -Werror (added by
microsoft#1415).
Wrapping the two defines in #ifndef in all three config-default*.h
headers preserves all existing behavior:
- Without command-line -D: macros get defined here as before.
- With command-line -D: header skips the redefinition, no warning.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…destruction
The modules-repo CI test ECSClientFuncTests.GetConfigs (and every
test in the ECSClientFuncTests suite) crashed on Linux/macOS with:
terminate called after throwing an instance of 'std::system_error'
what(): Resource deadlock avoided
Aborted (core dumped)
Root cause
==========
SendAsync() runs Send() + the user callback on a std::async worker
thread. The callback owns a strong ref to CurlHttpOperation, so when
it releases the last ref the ~CurlHttpOperation destructor runs on
the async thread itself.
libstdc++'s std::future<>::~future implicitly calls
_Async_state_impl::~_Async_state_impl, which calls _M_complete_async
-> _M_join via std::call_once. On the async thread that's a self-join;
call_once throws std::system_error(EDEADLK). Because the throw escapes
a noexcept destructor, terminate() aborts the process. A try/catch
around the future cannot rescue this — destructors of std::future are
noexcept.
Fix
===
Move the future onto a detached helper thread before its destructor
runs. The helper is by definition NOT the async thread (we'd only be
on the async thread if its work already finished), so the implicit
join completes immediately. On the common path (destruction from the
caller thread) it costs one short-lived thread spawn that exits in
microseconds.
Verified locally with sister + modules linked: all 113 FuncTests pass,
including all 25 ECSClientFuncTests (which include the formerly-fatal
GetConfigs).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ession)
Code review found that the previous fix detached the async future's join for
EVERY destruction. That removed the cross-thread lifetime guarantee the old
result.wait() provided: when the operation is destroyed from another thread
while the async Send() is still running, the destructor would proceed to
curl_easy_cleanup()/ReleaseResponse() and destroy the by-reference request body
while the worker thread is still using them -> use-after-free.
Restore the guarantee while keeping the EDEADLK self-join fix:
- Record the async task's thread id (atomic) when SendAsync's task starts.
- In the destructor, compare std::this_thread::get_id():
* self-join (destroyed from within our own async callback, e.g. EraseRequest
drops the last reference): the work is necessarily complete, so defer the
future's join to a detached helper thread instead of joining on this (the
async) thread, avoiding EDEADLK.
* cross-thread: result.wait() to keep the curl handle, response buffer and
by-reference request body alive until the async Send() finishes.
- Heap-allocate the deferred future first so a rare std::thread spawn failure
leaks the already-finished future rather than self-joining (EDEADLK) or
letting std::system_error escape this noexcept destructor (std::terminate).
- Refresh the stale HttpClient_Curl.cpp lifetime comment.
Logic validated with a standalone C++11 repro under AddressSanitizer: the
cross-thread path waits (no UAF) and the self-join path does not deadlock.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…row-new failure - Add #include <new> so std::nothrow is not relied on transitively (review). - If new (std::nothrow) returns nullptr (OOM), result stays valid and would self-join (EDEADLK) at end of the noexcept dtor; abort() as a last resort instead of falling through to that, per review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…fix lifetime comments - Replace std::atomic<std::thread::id> (not guaranteed supported across standard libraries) with a plain std::thread::id published via an std::atomic<bool> flag using release/acquire ordering. - Correct the lifetime comments: the operation's last shared_ptr is held by the owning CurlHttpRequest (via SetOperation), not by EraseRequest (which only removes the raw id from m_requests). The self-join occurs when the async callback leads to that request being destroyed on the async thread (OnHttpResponse -> EventsUploadContext::clear()). Re-validated the wait-vs-detach logic with a standalone C++11 repro under AddressSanitizer + UBSan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… reset flag on reuse - Reword the self-join comment: in that case Send() has returned (we are in its callback) but the async task itself has not yet returned (the destructor runs inside it), so the deferred helper's ~future join completes only after this destructor unwinds. Avoids implying the async task is already finished. - Reset m_asyncThreadIdSet to false at the start of SendAsync so self-join detection stays correct if the operation were ever reused (it is single-use today: one SendAsync per request). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…microsoft#1481) The EDEADLK self-join was a symptom of using std::async(std::launch::async) for the HTTP send: the returned std::future joins its worker thread on destruction, so when the async callback caused the operation to be destroyed on that same worker thread (OnHttpResponse -> EventsUploadContext::clear()), ~future self-joined and aborted the process out of the noexcept destructor. Rather than detect-and-defer that self-join (the previous approach: published thread id + atomic flag + heap-move the future to a detached helper, with OOM/ thread-exhaustion fallbacks), remove the joining future entirely: - CurlHttpOperation now derives from enable_shared_from_this. SendAsync runs Send() on a detached std::thread that holds a shared_ptr keepalive to the operation, so the operation (and its curl handle, response buffer, and by-reference request body) stays alive until the worker finishes -- the same lifetime guarantee the destructor's result.wait() used to provide. - There is no future, so ~CurlHttpOperation never joins anything and is safe on any thread, including the worker thread itself. The destructor drops to plain curl cleanup. - Removes the future member, the m_asyncThreadId/m_asyncThreadIdSet machinery, and the <future>/<new> includes. Net -54 lines in the client. Adds HttpClientCurlTests.SendAsync_DestroyOnWorkerThread_NoSelfJoin, which drops the last external reference from inside the callback (on the worker thread) -- the exact microsoft#1481 trigger. It aborts the process on the old std::async code and passes on this fix. Verified on Linux GCC 13: all HttpClientCurlTests (12) pass including the new regression; the full FuncTests suite (39) passes with the curl client. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… into bhamehta/fix-curl-async-self-join
…xceptions, tidy test - requestBody use-after-free (comments 1 & 3): the old blocking destructor kept the by-reference body alive because destroying the request waited for Send(). With the self-keepalive worker the operation can outlive the request, so a reference into CurlHttpRequest::m_body could dangle mid-send. CurlHttpOperation now takes the body by value and owns it, so it is valid for the operation's whole lifetime regardless of when the request is released. Costs one body copy per request (the prior zero-copy relied on the blocking wait that caused microsoft#1481). - Detached-worker exceptions (comment 2): an exception escaping Send()/callback would call std::terminate, whereas the old std::async captured (and effectively swallowed) it. Wrap the worker body in try/catch to preserve the non-terminating behavior. - Test (comment 4): replace the raw new/delete shared_ptr box with a shared_ptr<shared_ptr<CurlHttpOperation>> whose contained pointer is reset in the callback, so it cannot leak if SendAsync throws. Verified on Linux GCC 13: all HttpClientCurlTests (12) pass including the self-join regression; full FuncTests (39) pass with the by-value body. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…est host, tidy comment HttpClient_Curl.cpp:84 (comment 3547544648): the operation takes the request body by value, so hand it curlRequest->m_body via std::move instead of copying. m_body is a per-send copy of the EventsUploadContext body (the retry source of truth), so moving it is safe and avoids duplicating peak upload memory. HttpClientCurlTests.cpp:150 (comment 3547544635): replace the fixed port 9 URL with an RFC 6761 .invalid host so Send() fails fast and deterministically on any environment (a fixed port could happen to be open). connTimeout=1 still bounds it. HttpClient_Curl.hpp:183 (comment 3547544604): the destructor comment now says the request body is owned (by value), not by-reference, matching the current design. Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass (incl. SendAsync_DestroyOnWorkerThread_NoSelfJoin) and full FuncTests 39/39 pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… harden test promise HttpClient_Curl.hpp SendAsync (comment 3547753859): if std::thread creation throws (e.g. resource exhaustion) the exception previously escaped SendAsync(), which both violates the IHttpClient::SendRequestAsync contract that the callback is always invoked and, on the PAL worker thread (no try/catch), would terminate the process. The worker body is now a named lambda; thread start is wrapped in try/catch and on failure the operation runs synchronously as a fallback so the callback still fires and no exception escapes. HttpClientCurlTests.cpp (comment 3547753886): the regression test captured the stack std::promise by reference, so if the ASSERT timed out and the test returned early, the detached worker could call set_value() on a destroyed promise. The promise is now heap-owned (shared_ptr) and captured by value, so an early return cannot turn into a use-after-scope. Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass and FuncTests compiles clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…thread-start catch, fix test comment HttpClient_Curl.hpp (comment 3548205832): WaitOnSocket() uses std::numeric_limits but the header only included <numeric>, not <limits> -- it had relied on <future> (removed by this PR) to pull <limits> transitively. Added an explicit <limits> include so the header is self-contained. HttpClient_Curl.hpp SendAsync (comment 3548205850): the thread-start fallback only caught std::system_error, but std::thread construction can also throw std::bad_alloc while allocating the callable. Broadened the catch to const std::exception& so any thread-start failure still falls back to a synchronous run and never escapes SendAsync() (which would terminate on the PAL worker thread). HttpClientCurlTests.cpp (comment 3548205863): dropped the misleading "connTimeout=1 bounds it" note -- CurlHttpOperation ignores its httpConnTimeout arg (WaitOnSocket uses the HTTP_CONN_TIMEOUT constant), so the .invalid host's immediate name- resolution failure, not the timeout, is what makes Send() fail fast. Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n SendAsync Comment 3548251461: SendAsync() called shared_from_this() unconditionally. Every CurlHttpOperation is created via make_shared (HttpClient_Curl.cpp:89), so this is safe today, but if a future caller ever constructs one outside a shared_ptr (stack / unique_ptr) shared_from_this() throws std::bad_weak_ptr, which would escape SendAsync() BEFORE the thread-start try/catch and could terminate the caller thread -- breaking the "SendAsync never lets an exception escape / the callback is always invoked" property established in the earlier rounds. Guarded shared_from_this() with a std::bad_weak_ptr catch that falls back to a synchronous run (the caller owns the non-shared object for the duration). Also extracted the shared Send()+callback body into RunSendAndCallback() so the detached worker, the thread-start fallback, and this new no-shared fallback all use one implementation. Added regression test SendAsync_NotSharedOwned_RunsSynchronouslyNoThrow. Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…iew round 7) Comment 3548399891: the note claimed curlRequest->m_body was a "per-send copy of the EventsUploadContext body (the retry source of truth)". That's inaccurate -- the encoder MOVES ctx->body into the request (SimpleHttpRequest::SetBody does m_body = std::move(body), IHttpClient.hpp:310) and then clears ctx->body (HttpRequestEncoder.cpp:165-167), so m_body is the sole owner of the payload and ctx->body is not a retained retry buffer. Reworded to describe the actual ownership and why moving m_body is safe (the request is single-use and released with the EventsUploadContext). No code change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment 3548517158: on the (practically unreachable) 15s-timeout path the detached worker could still be running when the fixture tears down -- and the fixture holds HttpClient_Curl m_client (its dtor calls curl_global_cleanup) plus the m_headers/m_body the worker may still read -- risking a secondary crash unrelated to the regression. On timeout, best-effort cancel the still-running operation and wait briefly before failing, so the worker is much less likely to outlive teardown. The cancel handle is a std::weak_ptr so it does not keep the operation alive (an owning ref would defeat the test: the callback's box->reset() must remain the last external ref). Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass (NoSelfJoin normal path still ~45ms). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t#1481 review round 9) Comment 3548602231: the worker lambda was constructed before the try/catch. Copying callback (a std::function) into it can throw std::bad_alloc, which would escape SendAsync() despite the intent that any failure fall back to a synchronous run. Construct the lambda inline inside the std::thread() call within the try so a throwing capture-copy is caught alongside a thread-start failure; the catch now calls RunSendAndCallback(callback) directly (self keeps this operation alive for the synchronous run). This also drops the separate named worker variable. Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reword comments in the curl HTTP client and its tests to describe the behavior without citing tracking numbers; no code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot review: the fallback comments state the callback is 'always invoked', but RunSendAndCallback skipped the callback if Send() itself threw (the callback call was inside the same try). If Send() threw, the request was left outstanding and its IHttpClient callback never completed, which could hang the upload/cancel path. Restructured so Send() is guarded on its own, a thrown Send() sets a failure result (res = CURLE_FAILED_INIT), and the callback is then invoked unconditionally (itself guarded so a throwing callback can't escape the detached worker). The 'always invoked' contract now holds literally. Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The self-keepalive fix keeps the operation alive on the detached worker until Send() and the completion callback finish, so ~CurlHttpOperation can now run after the completion callback. In synchronous-handler builds (USE_SYNC_HTTPRESPONSE_HANDLER, which is defined by default) that callback runs HttpClientManager::onHttpResponse, which deletes the IHttpResponseCallback before returning. The destructor then dispatched OnDestroy through the now-dangling m_callback -- a use-after-free on every completed request (benign until the freed memory is reused; caught by ASAN). Track completion in an atomic flag set right after the completion callback runs, and skip the destructor's OnDestroy dispatch once completed. OnDestroy still fires when the operation is destroyed before completing (aborted, or a construction/dispatch failure), where m_callback is still valid. Add a regression test (SendAsync_NoOnDestroyDispatchAfterCompletion) that keeps the callback alive and asserts OnDestroy is not dispatched after completion; it fails without the guard and passes with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…test Follow-ups from code review of the completion-path UAF fix: - HttpClient_Curl.hpp uses std::move but relied on a transitive <utility>; include it directly. - The destructor comment claimed OnDestroy still fires on abort. It does not: every SendAsync path (including abort and the synchronous fallbacks) runs the completion callback and sets m_completed first, so OnDestroy is suppressed for any request that was actually sent. Correct the comment to say so. - Harden SendAsync_NoOnDestroyDispatchAfterCompletion: on the wait_for timeout path, abort the worker and wait so it can't outlive the stack frame whose cb/m_headers/m_body it reads; and let the destructor body finish before asserting so a missing guard is observed rather than raced past. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- SendRequestAsync moved the request body out of the request, but the request is read again after the send: HttpResponseDecoder emits the request payload on EVT_HTTP_OK / EVT_HTTP_ERROR when requestDone runs the decode chain. Moving it out left those debug events with an empty payload (a curl-only regression vs the WinInet and NSURLSession clients). Copy the body into the operation instead -- it still gets an owned buffer for the detached send, and the request keeps its body for the decoder. - ~HttpClient_Curl ran curl_global_cleanup, but detached workers run curl_easy_cleanup in ~CurlHttpOperation after the request callback has already been removed from HttpClientManager's tracking, so the shutdown drain could return before an operation's easy-handle cleanup finished -- curl_global_cleanup then races easy-handle cleanup (undefined behavior). Track in-flight operations and have ~HttpClient_Curl wait (bounded to 5s) for them before global cleanup. All 14 curl unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cleanup Address review findings on the async self-join fix: - Set m_completed after Send() regardless of whether a completion callback was provided. It was only set inside the non-null-callback branch, so a SendAsync() call with the default null callback left m_completed false and ~CurlHttpOperation would still DispatchEvent(OnDestroy) for a request that had actually been sent -- the use-after-free the guard exists to prevent. - Skip curl_global_cleanup() when the bounded in-flight drain times out. curl_global_cleanup must not run concurrently with the curl_easy_cleanup that in-flight operation destructors run on detached workers; proceeding after a timeout could crash. Leaking libcurl global state once at shutdown is the safer choice in that pathological case. Files: lib/http/HttpClient_Curl.hpp, lib/http/HttpClient_Curl.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Heap-own the TrackingCallback and capture the shared_ptr by value in the completion lambda so its lifetime is tied to the detached worker. Previously the stack callback was captured by reference: in the timeout/FAIL path the worker can still be running when the test returns, so it could read the callback after destruction (a use-after-free that could crash the whole test process). The final assertion now dereferences the shared_ptr. Files: tests/unittests/HttpClientCurlTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lback cases The destructor runs after the detached worker releases its reference only when Send() ran asynchronously; it also runs for operations that were never sent or when SendAsync fell back to a synchronous run. Destruction is safe on any thread in all cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
RunSendAndCallback sets m_completed regardless of the send result (including an immediate curl_easy_init failure), so OnDestroy is dispatched only when the operation is destroyed without SendAsync ever having run -- not on construction failure. Reword the comment to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t path Mirror the stronger teardown from SendAsync_NoOnDestroyDispatchAfterCompletion: on the (unexpected) timeout path, wait for weakOp to expire after Abort so the detached worker cannot outlive fixture teardown (m_client/curl_global_cleanup, m_headers, m_body) and cause secondary crashes that obscure the real failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Complete cancellation after WinHttpCloseHandle returns so HANDLE_CLOSING cannot dereference a destroyed request wrapper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Prevent detached UploadNow threads from outliving the functional test and racing later LogManager lifetimes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove completed requests before invoking application callbacks so concurrent teardown cannot destroy the wrapper while its terminal callback is still running. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Ensure vcpkg-built Apple libraries match the consumer deployment target and avoid linker warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4259-b03b-8eeb87c06837
Propagate the resolved iOS sysroot to embedding builds and keep Apple vendored targets compatible with strict warning settings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837
Remove legacy Apple architecture, platform, and deployment-target inputs so standalone scripts and embedding consumers share CMAKE_OSX_* configuration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837
Keep both selectable HTTP backends linked privately while dropping the unused Winsock dependency and headers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
…ult-windows-transport
Join upload workers before SDK teardown and cover in-flight cancellation with a deterministic HTTP test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
…-transport' into bhamehta/winhttp-default-windows-transport
Preserve the WinHTTP default transport and WinInet opt-in while adopting the modern CMake build layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Route callbacks through a weak request reference so late WinHTTP notifications cannot dereference a destroyed wrapper during teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Avoid external collector network delays so teardown behavior is reproducible in CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Use a closed localhost port instead of creating hundreds of concurrent fixture connections. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Expose the transport capability required by the upcoming cancellation-drain changes and correct the certificate-check documentation typo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Bring the async self-join and worker lifetime fixes into the WinHTTP transport branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Keep the shared interface and Visual Studio project ready for the upcoming PR 1494 merge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Port bounded pause cancellation and condition-variable callback draining so WinHTTP can use the upcoming manager contract without a merge conflict. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
OfflineStorageHandler::Flush() returned early when StartActivity() failed, which happens as soon as FlushAndTeardown() begins pausing the LogManager. That early return left m_flushPending == true and never posted m_flushComplete, so WaitForFlush() blocked forever and Shutdown() never completed. The race needs a flush to be pending when teardown starts, so it only reproduced when an earlier test had already pushed enough records to schedule an async flush -- which is why sendManyRequestsAndCancel hung in the full suite but passed in isolation. It was misread as WinHTTP cancellation not draining; the transport had already finished. Always release the waiters: cancel the pending handle, post the event, and clear the pending flag on the skipped path. Flush body moves to FlushImpl() so EndActivity() is paired with StartActivity() on exactly the path that acquired it. Verified on Windows: the doNothing/killIsTemporary/ sendManyRequestsAndCancel sequence that hung indefinitely now passes, 5/5 repeat runs are stable, functests are 43/43 and unittests 528/528. Files changed: lib/offline/OfflineStorageHandler.cpp lib/offline/OfflineStorageHandler.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
…-transport' into bhamehta/winhttp-default-windows-transport # Conflicts: # lib/http/HttpClientManager.cpp # lib/http/HttpClientManager.hpp # lib/system/TelemetrySystem.cpp
oneds_memcpy_s delegated straight to the CRT memcpy_s whenever _MSC_VER or __STDC_LIB_EXT1__ was defined, skipping its own constraint checks. On MSVC the CRT reports a constraint violation through the invalid parameter handler, whose default behaviour terminates the process via __fastfail (STATUS_STACK_BUFFER_OVERRUN / 0xC0000409) rather than returning EINVAL. This crashed AnnexKTests.memcpy_s in Debug builds, which Windows CI does run (test-win-latest.yml builds both Release and Debug). More importantly it was a latent abrupt-termination path in shipped Windows code: any caller passing count > destsz would kill the process instead of getting an error back. The delegate path also never zeroed the destination on error, contradicting the function's documented contract. Validate the arguments before copying on every platform so the documented "return EINVAL and zero the destination" behaviour holds uniformly. Also fix oneds_buffer_region_overlap, which used strict > against a one-past-the-last-byte address and so both missed genuine single-byte overlaps and mis-flagged merely adjacent buffers. Replaced with the standard half-open range test, with an explicit zero-length short circuit. Unit tests: 531/531 pass with no exclusions (previously the suite could not run AnnexKTests at all). Files changed: lib/utils/annex_k.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
CurlHttpOperation unconditionally set CURLOPT_HTTP_VERSION to CURL_HTTP_VERSION_2_0 with a comment claiming it would "fallback to HTTP/1.1 if not supported". libcurl does not do that: when the linked library was built without HTTP/2, requesting it fails the transfer with CURLE_UNSUPPORTED_PROTOCOL rather than negotiating down. On such a build every upload would fail. Add CurlHttpOperation::GetPreferredHttpVersion(), which probes curl_version_info for CURL_VERSION_HTTP2 and returns CURL_HTTP_VERSION_1_1 when HTTP/2 is unavailable, and use it at setopt time. This also fixes the Linux build. HttpClientCurlTests.cpp came in with the microsoft#1481 merge and calls GetPreferredHttpVersion(), which had no implementation, so UnitTests failed to compile and build-tests.sh then exited 127 on the missing binary in all three ubuntu legs. Files changed: lib/http/HttpClient_Curl.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
BasicFuncTests.sendManyRequestsAndCancel hung indefinitely on Win32 Release CI (54+ minutes against a ~10 minute baseline for the leg). WinHttpRequestWrapper::send() held m_requestsMutex across its entire body, including the synchronous-failure paths that call onRequestComplete(). onRequestComplete() invokes the application callback, which -- as the comment above that call already noted -- can synchronously tear down the client. That teardown reaches HttpClient_WinHttp::CancelAllRequests(), which waits on m_requestsCv. m_requestsMutex is a std::recursive_mutex and m_requestsCv is a std::condition_variable_any. condition_variable_any::wait() releases only ONE level of a recursive mutex, so waiting while the mutex was held twice left it locked. erase(), running on the WinHTTP callback thread, could then never acquire the mutex to remove the request and notify_all(), so the predicate never became true and the untimed wait never woke: a permanent lost-wakeup deadlock. The test provokes this by posting to closed port 127.0.0.1:1, which makes WinHttpSendRequest fail synchronously, and by setting CFG_INT_MAX_TEARDOWN_TIME = i % 2 so alternating iterations take the untimed full-shutdown wait. Split the handle-setup work into sendLocked(), which runs under the lock and only *reports* a synchronous failure, and send(), which completes the request via onRequestComplete() after the lock has been released. Cancellation is still serialized against setup, so a cancel cannot be lost mid-handle-creation. cancel() already called onRequestComplete() outside the lock and is unaffected. Verified on a CI-faithful Win32 Release MSBuild build (the MSBuild project compiles AISendTests/BondDecoderTests/EventDecoderListener, which the CMake build omits -- 44 tests from 5 suites vs 43 from 4 -- which is why earlier CMake-only runs did not reproduce it): - sendManyRequestsAndCancel: hung 54+ min -> passes in 16.9s - FuncTests 44/44 passed, no exclusions - UnitTests 501/501 passed, no exclusions Files changed: lib/http/HttpClient_WinHttp.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Temporary diagnostic branch used to capture native thread stacks for the BasicFuncTests.sendManyRequestsAndCancel hang seen only on the Win32-Release leg of PR #1520. Contains no SDK/product changes -- only a test-side watchdog that dumps thread stacks after a per-test deadline, full PDB generation for Win32 Release, and a reduced CI matrix. Will be closed once the stacks are captured.