open-telemetry / open-telemetry/opentelemetry-cpp

[BUG] BatchSpanProcessor::Shutdown ignores its timeout when the worker thread is blocked in Export()

Open
#4,532 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug triage/accepted
Dominant language
C++
Stars
1.4k
Forks
632
Avg merge
1d 13h
Merged PRs (30d)
75

Description

Body

Describe your environment

Reproduced on main at 11fa0db0 (also present in the latest release, v1.28.0 -- this is not a regression). Linux, built via the project's own CMake build (USE_HTTP_CLIENT_CURL, OTLP HTTP exporter enabled). Confirmed by reading sdk/src/trace/batch_span_processor.cc and by timing a standalone reproduction program against the built SDK.

Steps to reproduce

BatchSpanProcessor::InternalShutdown(timeout) joins the worker thread unconditionally, before it ever looks at timeout:

bool BatchSpanProcessor::InternalShutdown(std::chrono::microseconds timeout) noexcept
{
  auto start_time = std::chrono::system_clock::now();
  std::lock_guard<std::mutex> shutdown_guard{synchronization_data_->shutdown_m};
  bool already_shutdown = synchronization_data_->is_shutdown.exchange(true);

  if (worker_thread_.joinable())
  {
    {
      std::lock_guard<std::mutex> cv_lock(synchronization_data_->cv_m);
      synchronization_data_->is_force_wakeup_background_worker.store(true, std::memory_order_release);
      synchronization_data_->cv.notify_all();
    }
    worker_thread_.join();          // <-- unconditional, untimed
  }

  GetWaitAdjustedTime(timeout, start_time);   // <-- `timeout` is only consulted after the join returns
  if (!already_shutdown && exporter_ != nullptr)
  {
    return exporter_->Shutdown(timeout);
  }
  return true;
}

notify_all() only wakes the worker if it is currently parked in DoBackgroundWork()'s cv.wait_for(). If the worker is instead inside Export() -> exporter_->Export(...), blocked on a live network call to an unresponsive collector, the notification does nothing: the thread is busy, not waiting. worker_thread_.join() then blocks for however long that Export() call takes to fail.

DrainQueue(), called by the worker right before it exits, has the same problem -- it loops calling Export() for every remaining buffered batch with no deadline of its own:

void BatchSpanProcessor::DrainQueue()
{
  while (true)
  {
    if (buffer_.empty() && ...) break;
    Export();
  }
}

Minimal repro:

  1. Start a real TCP listener that accepts a connection and never responds (a "blackhole" collector).
  2. Point an OtlpHttpExporter at it (default options, so a 10s request timeout).
  3. Wrap it in a BatchSpanProcessor with a short schedule_delay_millis (e.g. 100ms).
  4. Feed the processor one span (OnStart/OnEnd), then sleep ~300ms so the periodic export is genuinely in flight against the blackhole.
  5. Call processor->Shutdown(std::chrono::microseconds(1)) and time it.

Happy to attach the full standalone program to the issue or a follow-up PR.

What is the expected behavior?

Shutdown(timeout) should return at or close to the requested timeout (here, ~1 microsecond), regardless of what the worker thread happens to be doing at the time.

What is the actual behavior?

Calling Shutdown(1 microsecond) now...
Shutdown() returned: true
Actual wall-clock time to return: 9702 ms

The 9702ms is entirely the OTLP HTTP exporter's own default 10s request timeout running its course -- the caller's timeout had no effect on it. Separately, Shutdown() returns true even though the export it was waiting on failed, so the return value doesn't reflect what happened either (same shape of gap as #4359 for the Elasticsearch exporter's Shutdown).

Additional context

Not exporter-specific: BatchSpanProcessor and BatchLogRecordProcessor (which share this structure) sit in front of every batching exporter -- OTLP HTTP, OTLP gRPC, Elasticsearch, etc. -- so any host application using batch export and expecting a bounded shutdown (graceful termination on SIGTERM, zero-downtime reload, etc.) is exposed regardless of which exporter is configured.

Distinct from #4359 (Elasticsearch exporter's own Shutdown ignores its timeout) and #4339 (OTLP gRPC ForceFlush returns on the first notification rather than checking its predicate) -- both of those are exporter-level. This one is in the processor layer that sits in front of every batching exporter, and is a different code path (the worker-thread join, not the exporter's own shutdown/flush logic).

The fix direction isn't entirely obvious, which is why I'm filing before attempting a patch: the straightforward-looking approach -- wait up to timeout for a completion signal from the worker, and stop waiting if it doesn't finish in time -- runs into worker_thread_ being a raw std::thread bound to this via &BatchSpanProcessor::DoBackgroundWork. If Shutdown()/~BatchSpanProcessor() gives up on the join before the worker actually exits, avoiding std::thread's destructor calling std::terminate() on a still-joinable thread means detach()-ing it -- but a detached thread still touching this->buffer_ / this->exporter_ after the processor object is destroyed is a new use-after-free, not a fix. A safe version likely needs the worker's state (buffer, exporter, synchronization data) to be reachable independent of the BatchSpanProcessor object's own lifetime, so a timed-out Shutdown() can return without the abandoned thread touching freed memory once it eventually does finish. That's a bigger structural change than the fix itself, so I'd rather confirm the shape a maintainer wants before sending a patch than guess.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in sdk/src/trace/batch_span_processor.cc, focusing on InternalShutdown(), DrainQueue(), and the worker entry point DoBackgroundWork(); compare the shared structure used by BatchLogRecordProcessor. Run the standalone blackhole-collector reproduction described in the issue to observe the untimed join. Done means shutdown respects its deadline without leaving a worker able to access destroyed processor state, with behavior covering the reported batch exporters.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
observability-sre
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.