tracing: Cancel active tracing requests upon destruction. (envoyproxy…

…#2260) When a LightStepTransporter gets destroyed, any active requests it has must be cancelled; otherwise callbacks will be invoked on a destructed object. Risk Level: Low Testing: Added a unit test to verify described behavior. Fixes envoyproxy#2257 Signed-off-by: Ryan Burn <[email protected]>
hecsalazar · Dec 30, 2017 · 907b3b7 · 907b3b7
1 parent 52af891
commit 907b3b7
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 3 deletions.
diff --git a/source/common/tracing/lightstep_tracer_impl.cc b/source/common/tracing/lightstep_tracer_impl.cc
@@ -39,6 +39,12 @@ class LightStepLogger : Logger::Loggable<Logger::Id::tracing> {
 LightStepDriver::LightStepTransporter::LightStepTransporter(LightStepDriver& driver)
     : driver_(driver) {}
 
+LightStepDriver::LightStepTransporter::~LightStepTransporter() {
+  if (active_request_ != nullptr) {
+    active_request_->cancel();
+  }
+}
+
 void LightStepDriver::LightStepTransporter::Send(const Protobuf::Message& request,
                                                  Protobuf::Message& response,
                                                  lightstep::AsyncTransporter::Callback& callback) {
@@ -53,13 +59,14 @@ void LightStepDriver::LightStepTransporter::Send(const Protobuf::Message& reques
 
   const uint64_t timeout =
       driver_.runtime().snapshot().getInteger("tracing.lightstep.request_timeout", 5000U);
-  driver_.clusterManager()
-      .httpAsyncClientForCluster(driver_.cluster()->name())
-      .send(std::move(message), *this, std::chrono::milliseconds(timeout));
+  active_request_ = driver_.clusterManager()
+                        .httpAsyncClientForCluster(driver_.cluster()->name())
+                        .send(std::move(message), *this, std::chrono::milliseconds(timeout));
 }
 
 void LightStepDriver::LightStepTransporter::onSuccess(Http::MessagePtr&& response) {
   try {
+    active_request_ = nullptr;
     Grpc::Common::validateResponse(*response);
 
     Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(),
@@ -80,6 +87,7 @@ void LightStepDriver::LightStepTransporter::onSuccess(Http::MessagePtr&& respons
 }
 
 void LightStepDriver::LightStepTransporter::onFailure(Http::AsyncClient::FailureReason) {
+  active_request_ = nullptr;
   Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(),
                            lightstep::CollectorMethodName(), false);
   active_callback_->OnFailure(std::make_error_code(std::errc::network_down));

diff --git a/source/common/tracing/lightstep_tracer_impl.h b/source/common/tracing/lightstep_tracer_impl.h
@@ -58,6 +58,8 @@ class LightStepDriver : public OpenTracingDriver {
   public:
     explicit LightStepTransporter(LightStepDriver& driver);
 
+    ~LightStepTransporter();
+
     // lightstep::AsyncTransporter
     void Send(const Protobuf::Message& request, Protobuf::Message& response,
               lightstep::AsyncTransporter::Callback& callback) override;
@@ -67,6 +69,7 @@ class LightStepDriver : public OpenTracingDriver {
     void onFailure(Http::AsyncClient::FailureReason) override;
 
   private:
+    Http::AsyncClient::Request* active_request_ = nullptr;
     lightstep::AsyncTransporter::Callback* active_callback_ = nullptr;
     Protobuf::Message* active_response_ = nullptr;
     LightStepDriver& driver_;

diff --git a/test/common/tracing/lightstep_tracer_impl_test.cc b/test/common/tracing/lightstep_tracer_impl_test.cc
@@ -320,6 +320,34 @@ TEST_F(LightStepDriverTest, FlushOneSpanGrpcFailure) {
   EXPECT_EQ(1U, stats_.counter("tracing.lightstep.spans_sent").value());
 }
 
+TEST_F(LightStepDriverTest, CancelRequestOnDestruction) {
+  setupValidDriver();
+
+  Http::MockAsyncClientRequest request(&cm_.async_client_);
+  Http::AsyncClient::Callbacks* callback;
+  const Optional<std::chrono::milliseconds> timeout(std::chrono::seconds(5));
+
+  EXPECT_CALL(cm_.async_client_, send_(_, _, timeout))
+      .WillOnce(
+          Invoke([&](Http::MessagePtr& /*message*/, Http::AsyncClient::Callbacks& callbacks,
+                     const Optional<std::chrono::milliseconds>&) -> Http::AsyncClient::Request* {
+            callback = &callbacks;
+
+            return &request;
+          }));
+  EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.min_flush_spans", 5))
+      .WillOnce(Return(1));
+  EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.lightstep.request_timeout", 5000U))
+      .WillOnce(Return(5000U));
+
+  SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_);
+  span->finishSpan();
+
+  EXPECT_CALL(request, cancel());
+
+  driver_.reset();
+}
+
 TEST_F(LightStepDriverTest, SerializeAndDeserializeContext) {
   for (OpenTracingDriver::PropagationMode propagation_mode :
        {OpenTracingDriver::PropagationMode::SingleHeader,