fix: prevent EDEADLK self-join in ~CurlHttpOperation on async-thread destruction by bmehta001 · Pull Request #1481 · microsoft/cpp_client_telemetry

bmehta001 · 2026-06-11T08:17:07Z

Problem

cpp_client_telemetry_modules (cpp_client_telemetry_modules) CI on Linux + macOS crashes during the first ECSClientFuncTests test with:

terminate called after throwing an instance of 'std::system_error'
  what():  Resource deadlock avoided
Aborted (core dumped)

Affects every PR that gets past compile.

Root cause

std::future<long> & SendAsync(std::function<void(CurlHttpOperation &)> callback = nullptr) {
    result = std::async(std::launch::async, [this, callback] {
        long result = Send();
        if (callback) callback(*this);   // <-- callback often holds a strong ref to *this
        return result;
    });
    return result;
}

The callback fired from the async thread is the last owner of the CurlHttpOperation. When it releases its shared_ptr, ~CurlHttpOperation runs on that same async thread.

~CurlHttpOperation then destroys its std::future<long> result member. libstdc++'s ~future -> ~_Async_state_impl calls _M_complete_async -> _M_join via std::call_once. Joining the async thread from the async thread is a self-join — call_once throws std::system_error("Resource deadlock avoided"). Because the throw escapes a noexcept destructor, terminate() aborts the process.

A try/catch around the future or even around an explicit result.wait() cannot rescue this: std::future's destructor itself is noexcept, so the throw lands at the noexcept boundary before any user catch can engage.

Fix

Remove the joining future entirely rather than working around its self-join.

std::async(std::launch::async) is the root of the problem: the std::future it returns joins its worker thread on destruction, so any path that destroys the operation on that worker thread self-joins. Instead of detecting that case and deferring the join, this drops std::async:

CurlHttpOperation now derives from std::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 and response buffer — stays alive until the worker finishes. That is 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 (where it runs when the callback drops the last other reference). The destructor collapses to plain curl cleanup.
The request body is copied and owned by the operation, not held by reference into the caller's IHttpRequest: because the detached worker can now outlive the request, a reference into the caller's buffer could dangle mid-send. The call site copies (rather than moves) the body on purpose: the request's m_body is read again after the send -- HttpResponseDecoder emits the request payload on EVT_HTTP_OK / EVT_HTTP_ERROR when requestDone runs the decode chain -- so moving it out would leave those debug events with an empty payload, a curl-only regression versus the WinInet and NSURLSession clients.
SendAsync is total — it never lets an exception escape and always invokes the IHttpClient callback. shared_from_this() (non-shared owner → std::bad_weak_ptr), the worker/callable construction (std::function copy → std::bad_alloc), and the thread start (std::system_error / std::bad_alloc) are all guarded; on any of these the operation runs synchronously on the caller's thread as a fallback.
Removes the result future member and the self-join-detection machinery.

auto self = shared_from_this();                       // guarded; sync fallback if not shared-owned
std::thread([self, callback]() {                      // guarded; sync fallback if start/alloc fails
    self->RunSendAndCallback(callback);               // runs Send() + callback, swallowing exceptions
    // self released here; ~CurlHttpOperation runs trivially (no future to join)
}).detach();

No API change; std::async already spawned one thread per send, so thread cost is unchanged.

Verification

Verified on Linux (GCC 13 + clang 18):

Regression test HttpClientCurlTests.SendAsync_DestroyOnWorkerThread_NoSelfJoin drops the last external reference from inside the callback (on the worker thread) — the exact destruction-on-worker-thread trigger. It aborts the process on the old std::async code and passes on this fix.
HttpClientCurlTests.SendAsync_NotSharedOwned_RunsSynchronouslyNoThrow covers the non-shared-owner synchronous fallback.
All HttpClientCurlTests (13) pass, plus the full FuncTests suite (39) with the curl client — no hang, no behavioral regression.

The original modules-CI repro (ECSClientFuncTests, which links the modules submodule) continues to be covered by that repo's CI; the new unit test reproduces the same destruction-on-worker-thread path directly here so it is guarded in this repo too.

Risk: low. One short-lived thread per send (unchanged from std::async); no API or behavior change.

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>

Copilot

Pull request overview

This PR addresses a Linux/macOS crash caused by std::future’s implicit join throwing on self-join when ~CurlHttpOperation runs on the same async thread created by std::async.

Changes:

Adds tracking of the async task’s thread id to detect destructor execution on the async thread.
Updates ~CurlHttpOperation to avoid self-join by deferring std::future destruction to a detached helper thread.
Updates comments in the curl HTTP client to document the lifetime/join behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
lib/http/HttpClient_Curl.hpp	Adds async thread id tracking and modifies destructor logic to avoid `std::future` self-join aborts.
lib/http/HttpClient_Curl.cpp	Updates documentation comment describing the lifetime guarantees of `CurlHttpOperation` across async send.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…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>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

…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>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

… 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>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

…nc-self-join

…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

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

…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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

…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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

…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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

…s path The callback sets the promise, but the detached worker still holds its self- reference until RunSendAndCallback returns. Wait (bounded) for weakOp to expire before the test returns so the operation's curl_easy_cleanup cannot race with fixture teardown, matching the other async regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

…ests Extract a shared DrainOperation helper that waits for the operation to be destroyed and aborts a stuck worker as a fallback, then hard-asserts it is gone. Both async regression tests now ensure the detached worker (and its curl_easy_cleanup) cannot outlive fixture teardown (m_client -> curl_global_cleanup) on either the success or timeout path, rather than returning while the worker might still run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

These directly-constructed operations are not tracked by HttpClient_Curl::m_activeOps, so nothing else bounds the race between a lingering worker's curl_easy_cleanup and the fixture's curl_global_cleanup. DrainOperationOrDie now aborts a stuck worker and, if the operation is still alive afterward (a genuine keepalive/abort regression), records a failure and std::abort()s rather than returning into fixture teardown with an in-flight curl worker. In practice the .invalid host fails DNS in milliseconds so the operation is always gone immediately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Move curl request tracking into shared state captured by detached workers so late callbacks do not dereference HttpClient_Curl after the shutdown drain times out. Preserve the bounded drain before curl_global_cleanup and abandon late callbacks/logging when shutdown cannot safely wait. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bmehta001 and others added 2 commits June 10, 2026 13:48

bmehta001 requested a review from a team as a code owner June 11, 2026 08:17

bmehta001 and others added 2 commits June 11, 2026 11:13

Merge branch 'main' into bhamehta/fix-curl-async-self-join

e76fabf

bmehta001 requested a review from Copilot June 13, 2026 22:00

Copilot started reviewing on behalf of bmehta001 June 13, 2026 22:00 View session

Copilot AI reviewed Jun 13, 2026

View reviewed changes

Comment thread lib/http/HttpClient_Curl.hpp

Comment thread lib/http/HttpClient_Curl.hpp Outdated

bmehta001 requested a review from Copilot June 13, 2026 22:24

Copilot started reviewing on behalf of bmehta001 June 13, 2026 22:24 View session

Copilot AI reviewed Jun 13, 2026

View reviewed changes

Comment thread lib/http/HttpClient_Curl.hpp Outdated

Comment thread lib/http/HttpClient_Curl.hpp Outdated

Comment thread lib/http/HttpClient_Curl.hpp Outdated

Comment thread lib/http/HttpClient_Curl.cpp Outdated

bmehta001 requested a review from Copilot June 13, 2026 22:40

Copilot started reviewing on behalf of bmehta001 June 13, 2026 22:40 View session

Copilot AI reviewed Jun 13, 2026

View reviewed changes

Comment thread lib/http/HttpClient_Curl.hpp Outdated

Comment thread lib/http/HttpClient_Curl.hpp Outdated

bmehta001 requested a review from Copilot June 13, 2026 22:50

Copilot started reviewing on behalf of bmehta001 June 13, 2026 22:50 View session

Copilot AI reviewed Jun 13, 2026

View reviewed changes

bmehta001 added 2 commits June 29, 2026 22:56

Merge branch 'main' into bhamehta/fix-curl-async-self-join

dcbc2a2

Merge branch 'main' into bhamehta/fix-curl-async-self-join

7a14e76

bmehta001 self-assigned this Jul 8, 2026

bmehta001 and others added 3 commits July 8, 2026 15:58

Merge remote-tracking branch 'origin/main' into bhamehta/fix-curl-asy…

6bf8f55

…nc-self-join

Merge remote-tracking branch 'fork/bhamehta/fix-curl-async-self-join'…

854f2dd

… into bhamehta/fix-curl-async-self-join

bmehta001 requested a review from Copilot July 8, 2026 21:31

Copilot started reviewing on behalf of bmehta001 July 8, 2026 21:31 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread lib/http/HttpClient_Curl.hpp Outdated

Comment thread lib/http/HttpClient_Curl.hpp Outdated

Comment thread lib/http/HttpClient_Curl.cpp

Comment thread tests/unittests/HttpClientCurlTests.cpp Outdated