Close a race by checking for repeated End() calls in SpanImpl. (#116)

Inline EndWithTime() into its one caller.

Author: g-easy

opencensus/trace/internal/span.cc

@@ -202,12 +202,10 @@ void Span::SetStatus(StatusCode canonical_code, absl::string_view message) {
 
 void Span::End() {
   if (IsRecording()) {
-    if (span_impl_->HasEnded()) {
-      assert(false && "Invalid attempt to End() the same Span more than once.");
+    if (!span_impl_->End()) {
       // In non-debug builds, ignore the second End().
       return;
     }
-    span_impl_->End();
     exporter::RunningSpanStoreImpl::Get()->RemoveSpan(span_impl_);
     exporter::LocalSpanStoreImpl::Get()->AddSpan(span_impl_);
     exporter::SpanExporterImpl::Get()->AddSpan(span_impl_);

opencensus/trace/internal/span_impl.cc

@@ -137,19 +137,21 @@ void SpanImpl::SetStatus(exporter::Status&& status) {
   }
 }
 
-void SpanImpl::End() { EndWithTime(absl::Now()); }
-
-bool SpanImpl::HasEnded() const {
+bool SpanImpl::End() {
   absl::MutexLock l(&mu_);
-  return has_ended_;
+  if (has_ended_) {
+    assert(false && "Invalid attempt to End() the same Span more than once.");
+    // In non-debug builds, just ignore the second End().
+    return false;
+  }
+  has_ended_ = true;
+  end_time_ = absl::Now();
+  return true;
 }
 
-void SpanImpl::EndWithTime(absl::Time end_time) {
+bool SpanImpl::HasEnded() const {
   absl::MutexLock l(&mu_);
-  if (!has_ended_) {
-    has_ended_ = true;
-    end_time_ = end_time;
-  }
+  return has_ended_;
 }
 
 exporter::SpanData SpanImpl::ToSpanData() const {

opencensus/trace/internal/span_impl.h

@@ -85,8 +85,10 @@ class SpanImpl final {
 
   void SetStatus(exporter::Status&& status) LOCKS_EXCLUDED(mu_);
 
-  // Marks the end of the Span and sets its end_time_.
-  void End() LOCKS_EXCLUDED(mu_);
+  // Returns true on success (if this is the first time the Span has ended) and
+  // also marks the end of the Span and sets its end_time_.
+  bool End() LOCKS_EXCLUDED(mu_);
+
   // Returns true if the span has ended.
   bool HasEnded() const LOCKS_EXCLUDED(mu_);
 
@@ -106,8 +108,6 @@ class SpanImpl final {
   friend class ::opencensus::trace::exporter::SpanExporterImpl;
   friend class ::opencensus::trace::SpanTestPeer;
 
-  void EndWithTime(absl::Time end_time) LOCKS_EXCLUDED(mu_);
-
   // Makes a deep copy of span contents and returns copied data in SpanData.
   exporter::SpanData ToSpanData() const LOCKS_EXCLUDED(mu_);