Hacktoberfest 2026: le issue che i maintainer hanno segnato per ottobre, aperte e adatte ai principianti. Sfoglia le issue Hacktoberfest

[BUG] A send during WaitBackgroundThreadExit starts a second background thread, and teardown races it

Aperta
  • Qualcosa non va?
#4,665 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
38/100
Tipo di issue
Bug
Chiarezza
Abbastanza chiara
Stato di attività
Attiva
Stack tecnologico
cmake, cpp
Ambito
networking

Direzione di ricerca

Start in http_client_curl.cc at MaybeSpawnBackgroundThread(), WaitBackgroundThreadExit(), the destructor loop around lines 322-342, and the self-detach path near line 654; compare these lifecycle paths with ext/test/http/curl_http_test.cc. Run the named HTTP curl tests with thread instrumentation and ThreadSanitizer, using the gated AfterWait ordering described. Done means the wait leaves no client background thread running and teardown produces no curl_multi_cleanup race.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

needs-triage

Describe your environment

  • WaitBackgroundThreadExit() came in with #3198 and first shipped in v1.19.0. The re-swap loop it is missing has been in the destructor since #1413 and v1.5.0. main at 489cd823 still has both shapes.
  • Ubuntu 24.04, gcc 14, libcurl 8.14.1, CMake, built with OTELCPP_WITH_THREAD_INSTRUMENTATION_PREVIEW=ON. That flag is what makes the ordering below deterministic instead of timed; the code it exercises is not behind it and is not platform specific.

Steps to reproduce

  1. Give an ext::http::client::curl::HttpClient a ThreadInstrumentation whose AfterWait() blocks until the test releases it. AfterWait is called at http_client_curl.cc:525 and holds no lock. OnEnd will not do for this: it runs at :648, inside the guard taken at :612, which is the mutex the spawn needs, so blocking there blocks the spawn instead of letting it through.
  2. Call MaybeSpawnBackgroundThread() and wait until the thread is parked in AfterWait.
  3. From another thread, call WaitBackgroundThreadExit(). It swaps background_thread_ out at :723, releases the lock at :724, and blocks in the join at :729 because the thread it took is parked.
  4. Send a request. Session::SendRequest calls MaybeSpawnBackgroundThread() at :258, it finds a null member, and a second thread starts. Point the request at an address that does not answer, https://192.0.2.0:19000/get/ as SendGetRequestAsyncTimeout already does, so the new thread has a session to service rather than nothing to do.
  5. Release the parked thread, join the caller of WaitBackgroundThreadExit(), and count the background threads that have started and not ended.

What is the expected behavior?

Once WaitBackgroundThreadExit() returns, no background thread of that client is running. The destructor already behaves that way. Lines :322 to :342 swap and join in a loop until a swap comes back empty, so a thread that appears during teardown is collected on the next turn.

What is the actual behavior?

The wait joins the thread it swapped out and returns whether or not another has appeared:

719:  is_shutdown_.store(true, ...);
722:    std::lock_guard<std::mutex> lock_guard{background_thread_m_};
723:    background_thread.swap(background_thread_);    <- member is null from here
724:  }                                               <- lock released
729:    background_thread->join();                    <- joins only what it swapped out
731:  is_shutdown_.store(false, ...);

MaybeSpawnBackgroundThread() takes the same mutex at :471 and then tests background_thread_ at :472 and nothing else. From :724 to :729 that member is null, so the test passes and a thread starts. is_shutdown_ is true for the whole span and the spawn never reads it.

Nor does the new thread simply retire again. With a session queued, doAddSessions() at :629 sets still_running and it stays in the loop. Five runs out of five, with the ordering forced by the gate rather than by timing: two threads started, and one of them was still in its loop at the moment the wait returned.

Teardown then races it. Finishing the session, calling FinishAllSessions() and then destroying the client, which is what a caller would do next, gives ThreadSanitizer this, three runs out of three:

SUMMARY: ThreadSanitizer: data race in curl_multi_cleanup
  Write of size 8 by main thread (mutexes: write M0):
    curl_multi_cleanup                       <- from ~HttpClient
  Previous read of size 8 by thread T4:
    libcurl.so.4
  Location is file descriptor 5 created by main thread
  Thread T4 created by main thread at:
    Session::SendRequest(...) http_client_curl.cc:258

T4 is the thread the send started. All 16 reports in that run have the main thread on one side and T4 on the other, and 13 of them name ~HttpClient.

Additional context

The race needs both halves, so here is the grid rather than the one cell that fails. Each row is the same TSan build, run on its own:

scenario window session in flight races
SendGetRequestAsyncTimeout, unchanged no yes, same unreachable address 0, 3 of 3
ElegantQuitQuick, unchanged no yes, and it destroys with a thread live 0, 3 of 3
the window held open, with no session queued yes no 0
the recipe above yes yes 16, 16, 3

Two existing cases already destroy a client while a background thread runs, and they are clean. Neither the window alone nor the work alone produces anything.

What I have not instrumented is how the second thread gets past the destructor's join loop. My reading is the self-detach at :654: the thread retires, finds its own handle in background_thread_, detaches and clears it, and the loop at :322 then swaps out an empty pointer and breaks. That is a reading of the code, not a measurement, and I would rather hand it over as one.

Nothing in this repository calls WaitBackgroundThreadExit() outside ext/test/http/curl_http_test.cc, where it is called six times, so no shipped code path reaches this today. It is public API.

This is not #4402. That one parks the single IO thread from inside a handler. This is the opposite: a second IO thread where the design allows one.

None of #4395, #4405 or #4406 covers it. They change the loop body and the retry path, and leave the swap at :723 and the test at :472 as they are.

#4664 is the other open report on this loop, filed the same day: the CURLMcode from curl_multi_poll at :513 is stored and never read. Different defect, same function.

I would like to take it. The destructor's loop is the shape that already works here; the other option is for MaybeSpawnBackgroundThread() to refuse while is_shutdown_ is set. The second changes what a send does during shutdown, so it is worth a word from you before I write either one.

Tip: React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.

Lingua principale
C++
Stelle
1.4k
Fork
647
Merge medio
1g 10h
PR unite (30g)
73

Preparare l'ambiente

Apri in Codespaces

Avvia il container di sviluppo del progetto nel browser, con il tuo account GitHub.

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di open-telemetry/opentelemetry-cpp

Tutte le issue di open-telemetry/opentelemetry-cpp

Issue simili

Altre issue su C++

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.