[SDK] Release provider locks before construction-failure diagnostics (#4702)

Author: 22elix3r

CHANGELOG.md

@@ -15,6 +15,11 @@ Increment the:
 
 ## [Unreleased]
 
+* [SDK] Release provider mutexes before construction-failure diagnostics so
+  custom log handlers can retrieve cached or noop tracers, loggers, and meters
+  from the same provider without deadlocking.
+  [#4693](https://github.com/open-telemetry/opentelemetry-cpp/issues/4693)
+
 * [SDK] Preserve the default unlimited timeout when batch span and log record
   processors shut down their exporters.
   [#4686](https://github.com/open-telemetry/opentelemetry-cpp/pull/4686)

sdk/src/logs/logger_provider.cc

@@ -116,34 +116,35 @@ opentelemetry::nostd::shared_ptr<opentelemetry::logs::Logger> LoggerProvider::Ge
     name = logger_name;
   }
 
-  // Ensure only one thread can read/write from the map of loggers
-  std::lock_guard<std::mutex> lock_guard{lock_};
-
-  // If a logger with a name "logger_name" already exists, return it
-  for (auto &logger : loggers_)
+#if OPENTELEMETRY_HAVE_EXCEPTIONS
+  try
   {
-    auto &logger_lib = logger->GetInstrumentationScope();
-    if (logger->GetName() == logger_name &&
-        logger_lib.equal(name, version, schema_url, &attributes))
+#endif
+    // Release the lock before construction-failure diagnostics can re-enter the provider.
+    // Ensure only one thread can read/write from the map of loggers
+    std::lock_guard<std::mutex> lock_guard{lock_};
+
+    // If a logger with a name "logger_name" already exists, return it
+    for (auto &logger : loggers_)
     {
-      return opentelemetry::nostd::shared_ptr<opentelemetry::logs::Logger>{logger};
+      auto &logger_lib = logger->GetInstrumentationScope();
+      if (logger->GetName() == logger_name &&
+          logger_lib.equal(name, version, schema_url, &attributes))
+      {
+        return opentelemetry::nostd::shared_ptr<opentelemetry::logs::Logger>{logger};
+      }
     }
-  }
 
-  // GetLogger is noexcept and does not surface an error, so callers cannot
-  // recover after a construction failure. Retrying would re-throw, catch, and
-  // log on later GetLogger calls. After the first failure, treat the provider
-  // as non-functional for new loggers.
-  if (!initialized_)
-  {
-    return noop_logger_;
-  }
+    // GetLogger is noexcept and does not surface an error, so callers cannot
+    // recover after a construction failure. Retrying would re-throw, catch, and
+    // log on later GetLogger calls. After the first failure, treat the provider
+    // as non-functional for new loggers.
+    if (!initialized_)
+    {
+      return noop_logger_;
+    }
 
-  initialized_ = false;
-#if OPENTELEMETRY_HAVE_EXCEPTIONS
-  try
-  {
-#endif
+    initialized_ = false;
     std::unique_ptr<instrumentationscope::InstrumentationScope> lib =
         instrumentationscope::InstrumentationScope::Create(name, version, schema_url, attributes);
 

sdk/src/metrics/meter_provider.cc

@@ -122,31 +122,32 @@ nostd::shared_ptr<metrics_api::Meter> MeterProvider::GetMeter(
     name = "";
   }
 
-  const std::lock_guard<std::mutex> guard(lock_);
-
-  for (auto &meter : context_->GetMeters())
+#if OPENTELEMETRY_HAVE_EXCEPTIONS
+  try
   {
-    auto meter_lib = meter->GetInstrumentationScope();
-    if (meter_lib->equal(name, version, schema_url, attributes))
+#endif
+    // Release the lock before construction-failure diagnostics can re-enter the provider.
+    const std::lock_guard<std::mutex> guard(lock_);
+
+    for (auto &meter : context_->GetMeters())
     {
-      return nostd::shared_ptr<metrics_api::Meter>{meter};
+      auto meter_lib = meter->GetInstrumentationScope();
+      if (meter_lib->equal(name, version, schema_url, attributes))
+      {
+        return nostd::shared_ptr<metrics_api::Meter>{meter};
+      }
     }
-  }
 
-  // GetMeter is noexcept and does not surface an error, so callers cannot
-  // recover after a construction failure. Retrying would re-throw, catch, and
-  // log on later GetMeter calls. After the first failure, treat the provider
-  // as non-functional for new meters.
-  if (!initialized_)
-  {
-    return noop_meter_;
-  }
+    // GetMeter is noexcept and does not surface an error, so callers cannot
+    // recover after a construction failure. Retrying would re-throw, catch, and
+    // log on later GetMeter calls. After the first failure, treat the provider
+    // as non-functional for new meters.
+    if (!initialized_)
+    {
+      return noop_meter_;
+    }
 
-  initialized_ = false;
-#if OPENTELEMETRY_HAVE_EXCEPTIONS
-  try
-  {
-#endif
+    initialized_ = false;
     instrumentationscope::InstrumentationScopeAttributes attrs_map(attributes);
     auto scope =
         instrumentationscope::InstrumentationScope::Create(name, version, schema_url, attrs_map);

sdk/src/trace/tracer_provider.cc

@@ -146,31 +146,32 @@ nostd::shared_ptr<trace_api::Tracer> TracerProvider::GetTracer(
     OTEL_INTERNAL_LOG_ERROR("[TracerProvider::GetTracer] Library name is empty.");
   }
 
-  const std::lock_guard<std::mutex> guard(lock_);
-
-  for (auto &tracer : tracers_)
+#if OPENTELEMETRY_HAVE_EXCEPTIONS
+  try
   {
-    auto &tracer_scope = tracer->GetInstrumentationScope();
-    if (tracer_scope.equal(name, version, schema_url, attributes))
+#endif
+    // Release the lock before construction-failure diagnostics can re-enter the provider.
+    const std::lock_guard<std::mutex> guard(lock_);
+
+    for (auto &tracer : tracers_)
     {
-      return nostd::shared_ptr<trace_api::Tracer>{tracer};
+      auto &tracer_scope = tracer->GetInstrumentationScope();
+      if (tracer_scope.equal(name, version, schema_url, attributes))
+      {
+        return nostd::shared_ptr<trace_api::Tracer>{tracer};
+      }
     }
-  }
 
-  // GetTracer is noexcept and does not surface an error, so callers cannot
-  // recover after a construction failure. Retrying would re-throw, catch, and
-  // log on hot paths such as GetTracer(...)->StartSpan(...). After the first
-  // failure, treat the provider as non-functional for new tracers.
-  if (!initialized_)
-  {
-    return noop_tracer_;
-  }
+    // GetTracer is noexcept and does not surface an error, so callers cannot
+    // recover after a construction failure. Retrying would re-throw, catch, and
+    // log on hot paths such as GetTracer(...)->StartSpan(...). After the first
+    // failure, treat the provider as non-functional for new tracers.
+    if (!initialized_)
+    {
+      return noop_tracer_;
+    }
 
-  initialized_ = false;
-#if OPENTELEMETRY_HAVE_EXCEPTIONS
-  try
-  {
-#endif
+    initialized_ = false;
     instrumentationscope::InstrumentationScopeAttributes attrs_map(attributes);
     auto scope =
         instrumentationscope::InstrumentationScope::Create(name, version, schema_url, attrs_map);

sdk/test/logs/BUILD

@@ -15,6 +15,7 @@ cc_test(
     deps = [
         "//api",
         "//sdk/src/logs",
+        "//test_common:headers",
         "@com_google_googletest//:gtest_main",
     ],
 )

sdk/test/logs/CMakeLists.txt

@@ -17,8 +17,15 @@ foreach(
   target_link_libraries(
     ${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
     opentelemetry_logs opentelemetry_trace)
+  if(testname STREQUAL "logger_provider_sdk_test")
+    target_link_libraries(${testname} opentelemetry_test_common)
+  endif()
   gtest_add_tests(
     TARGET ${testname}
     TEST_PREFIX logs.
     TEST_LIST ${testname})
 endforeach()
+
+set_tests_properties(
+  logs.LoggerProviderSDK.ConstructionFailureDiagnosticsAllowProviderReentry
+  PROPERTIES TIMEOUT 10)

sdk/test/logs/logger_provider_sdk_test.cc

@@ -8,6 +8,7 @@
 #include <cstdint>
 #include <functional>
 #include <future>
+#include <memory>  // IWYU pragma: keep
 #include <mutex>
 #include <stdexcept>
 #include <string>
@@ -29,6 +30,7 @@
 #include "opentelemetry/nostd/utility.h"
 #include "opentelemetry/nostd/variant.h"
 #include "opentelemetry/sdk/common/exporter_utils.h"
+#include "opentelemetry/sdk/common/global_log_handler.h"
 #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h"
 #include "opentelemetry/sdk/instrumentationscope/scope_configurator.h"
 #include "opentelemetry/sdk/logs/event_logger_provider.h"          // IWYU pragma: keep
@@ -44,6 +46,7 @@
 #include "opentelemetry/sdk/logs/recordable.h"
 #include "opentelemetry/sdk/logs/simple_log_record_processor.h"
 #include "opentelemetry/sdk/resource/resource.h"
+#include "opentelemetry/test_common/sdk/common/scoped_test_log_handler.h"
 
 using namespace opentelemetry::sdk::logs;
 namespace logs_api  = opentelemetry::logs;
@@ -679,6 +682,61 @@ TEST(LoggerProviderSDK, ConstructorsAreNotNoexcept)
 }
 
 #if OPENTELEMETRY_HAVE_EXCEPTIONS
+TEST(LoggerProviderSDK, ConstructionFailureDiagnosticsAllowProviderReentry)
+{
+  using opentelemetry::sdk::common::internal_log::LogLevel;
+  using opentelemetry::test_common::ScopedTestLogHandler;
+  for (int exception_type = 0; exception_type < 2; ++exception_type)
+  {
+    SCOPED_TRACE(exception_type);
+    int construct_attempts = 0;
+    auto configurator      = std::make_unique<scope_sdk::ScopeConfigurator<LoggerConfig>>(
+        scope_sdk::ScopeConfigurator<LoggerConfig>::Builder(LoggerConfig::Default())
+            .AddCondition(
+                [&](const scope_sdk::InstrumentationScope &scope) {
+                  ++construct_attempts;
+                  if (scope.GetName() == "throwing-scope")
+                  {
+                    if (exception_type == 0)
+                    {
+                      throw std::runtime_error("injected construction failure");
+                    }
+                    throw 1;
+                  }
+                  return false;
+                },
+                LoggerConfig::Default())
+            .Build());
+    LoggerProvider provider(std::vector<std::unique_ptr<LogRecordProcessor>>{},
+                            opentelemetry::sdk::resource::Resource::Create({}),
+                            std::move(configurator));
+    auto cached = provider.GetLogger("cached-logger", "cached-scope");
+    nostd::shared_ptr<logs_api::Logger> reentrant_noop;
+    int diagnostics = 0;
+    ScopedTestLogHandler handler(LogLevel::Error, [&](const ScopedTestLogHandler::Entry &entry) {
+      ++diagnostics;
+      EXPECT_EQ(entry.level, LogLevel::Error);
+      EXPECT_EQ(provider.GetLogger("cached-logger", "cached-scope"), cached);
+      reentrant_noop = provider.GetLogger("uncached-logger", "uncached-scope");
+    });
+
+    auto failed = provider.GetLogger("throwing-logger", "throwing-scope");
+    ASSERT_NE(failed, nullptr);
+    EXPECT_EQ(diagnostics, 1);
+    EXPECT_EQ(reentrant_noop, failed);
+    EXPECT_EQ(failed->GetName(), "noop logger");
+    EXPECT_EQ(provider.GetLogger("cached-logger", "cached-scope"), cached);
+    EXPECT_EQ(provider.GetLogger("another-logger", "another-scope"), failed);
+    EXPECT_EQ(construct_attempts, 2);
+    EXPECT_EQ(diagnostics, 1);
+    auto entries = handler.Drain();
+    ASSERT_EQ(entries.size(), 1u);
+    EXPECT_NE(entries[0].msg.find(exception_type == 0 ? "injected construction failure"
+                                                      : "unknown exception"),
+              std::string::npos);
+  }
+}
+
 TEST(LoggerProviderSDK, GetLoggerReturnsNoopOnConstructionFailure)
 {
   auto should_throw          = std::make_shared<bool>(true);

sdk/test/metrics/CMakeLists.txt

@@ -52,6 +52,10 @@ foreach(
     TEST_LIST ${testname})
 endforeach()
 
+set_tests_properties(
+  metrics.MeterProvider.ConstructionFailureDiagnosticsAllowProviderReentry
+  PROPERTIES TIMEOUT 10)
+
 if(OPENTELEMETRY_ABI_VERSION_NO GREATER_EQUAL 2)
   add_executable(multi_observer_test multi_observer_test.cc)
   target_link_libraries(

sdk/test/metrics/meter_provider_sdk_test.cc

@@ -3,7 +3,7 @@
 
 #include <gtest/gtest.h>
 #include <functional>
-#include <memory>
+#include <memory>  // IWYU pragma: keep
 #include <stdexcept>
 #include <string>
 #include <utility>
@@ -17,10 +17,12 @@
 #include "opentelemetry/common/macros.h"
 #include "opentelemetry/metrics/meter.h"
 #include "opentelemetry/metrics/sync_instruments.h"
+#include "opentelemetry/nostd/shared_ptr.h"
 #include "opentelemetry/nostd/string_view.h"
 #include "opentelemetry/sdk/common/global_log_handler.h"
 #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h"
 #include "opentelemetry/sdk/instrumentationscope/scope_configurator.h"
+#include "opentelemetry/sdk/metrics/export/metric_producer.h"
 #include "opentelemetry/sdk/metrics/instruments.h"
 #include "opentelemetry/sdk/metrics/meter.h"
 #include "opentelemetry/sdk/metrics/meter_config.h"
@@ -531,6 +533,60 @@ TEST(MeterProvider, ConstructorsAreNotNoexcept)
 }
 
 #if OPENTELEMETRY_HAVE_EXCEPTIONS
+TEST(MeterProvider, ConstructionFailureDiagnosticsAllowProviderReentry)
+{
+  using opentelemetry::sdk::instrumentationscope::InstrumentationScope;
+  using opentelemetry::sdk::instrumentationscope::ScopeConfigurator;
+  for (int exception_type = 0; exception_type < 2; ++exception_type)
+  {
+    SCOPED_TRACE(exception_type);
+    int construct_attempts = 0;
+    auto configurator      = std::make_unique<ScopeConfigurator<MeterConfig>>(
+        ScopeConfigurator<MeterConfig>::Builder(MeterConfig::Default())
+            .AddCondition(
+                [&](const InstrumentationScope &scope) {
+                  ++construct_attempts;
+                  if (scope.GetName() == "throwing-scope")
+                  {
+                    if (exception_type == 0)
+                    {
+                      throw std::runtime_error("injected construction failure");
+                    }
+                    throw 1;
+                  }
+                  return false;
+                },
+                MeterConfig::Default())
+            .Build());
+    MeterProvider provider(std::make_unique<ViewRegistry>(),
+                           opentelemetry::sdk::resource::Resource::Create({}),
+                           std::move(configurator));
+    auto cached = provider.GetMeter("cached-scope");
+    opentelemetry::nostd::shared_ptr<opentelemetry::metrics::Meter> reentrant_noop;
+    int diagnostics = 0;
+    ScopedTestLogHandler handler(LogLevel::Error, [&](const ScopedTestLogHandler::Entry &entry) {
+      ++diagnostics;
+      EXPECT_EQ(entry.level, LogLevel::Error);
+      EXPECT_EQ(provider.GetMeter("cached-scope"), cached);
+      reentrant_noop = provider.GetMeter("uncached-scope");
+    });
+
+    auto failed = provider.GetMeter("throwing-scope");
+    ASSERT_NE(failed, nullptr);
+    EXPECT_EQ(diagnostics, 1);
+    EXPECT_EQ(reentrant_noop, failed);
+    EXPECT_EQ(provider.GetMeter("cached-scope"), cached);
+    EXPECT_EQ(provider.GetMeter("another-scope"), failed);
+    EXPECT_EQ(construct_attempts, 2);
+    EXPECT_EQ(diagnostics, 1);
+    auto entries = handler.Drain();
+    ASSERT_EQ(entries.size(), 1u);
+    EXPECT_NE(entries[0].msg.find(exception_type == 0 ? "injected construction failure"
+                                                      : "unknown exception"),
+              std::string::npos);
+  }
+}
+
 TEST(MeterProvider, GetMeterReturnsNoopOnConstructionFailure)
 {
   auto should_throw       = std::make_shared<bool>(true);

sdk/test/trace/BUILD

@@ -17,6 +17,7 @@ cc_test(
         "//exporters/memory:in_memory_span_exporter",
         "//sdk/src/resource",
         "//sdk/src/trace",
+        "//test_common:headers",
         "@com_google_googletest//:gtest_main",
     ],
 )