From f3b5a978691c4c0e22e057a1052dad6f8b541895 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 15 Oct 2021 11:16:50 -0700 Subject: [PATCH 1/5] Fix flaky test TestSimpleSpanProcessorShutdownHonorsContextCancel --- sdk/trace/simple_span_processor.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sdk/trace/simple_span_processor.go b/sdk/trace/simple_span_processor.go index 9dc87f893b9..32ea7afa643 100644 --- a/sdk/trace/simple_span_processor.go +++ b/sdk/trace/simple_span_processor.go @@ -90,6 +90,16 @@ func (ssp *simpleSpanProcessor) Shutdown(ctx context.Context) error { select { case err = <-done: + // It is possible for the exporter to have immediately shut down and + // the context to already be done. In that case this select statement + // will randomly choose a case. This could result in a different + // response for similar scenarios. Instead, double check if the + // context is done here and return that error if the exporter shut + // down did not return one. + if err == nil { + // If ctx is not done yet, ctx.Err returns nil. + err = ctx.Err() + } case <-ctx.Done(): err = ctx.Err() } From 0b421c563d3ca9014ade72424c2fb3002ca11c07 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 15 Oct 2021 11:21:13 -0700 Subject: [PATCH 2/5] Add changes to changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fcfa23c942..e43d7c6d84f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Adds `otlptracegrpc.WithGRPCConn` and `otlpmetricgrpc.WithGRPCConn` for reusing existing gRPC connection. (#2002) - Added a new `schema` module to help parse Schema Files in OTEP 0152 format. (#2267) +### Fixed + +- The simple span processor shutdown method returns any context error if there is one and the exporter successfully shuts down. (#2290, #2289) + ## [1.0.1] - 2021-10-01 ### Fixed From a0e2d0b706ad742e1693a910cfb4ce2e08cf789e Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 18 Oct 2021 10:31:53 -0700 Subject: [PATCH 3/5] Prioritize return of exporter error --- sdk/trace/simple_span_processor.go | 20 ++++++++++---------- sdk/trace/simple_span_processor_test.go | 10 ++++++++-- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/sdk/trace/simple_span_processor.go b/sdk/trace/simple_span_processor.go index 32ea7afa643..5efe50a9bff 100644 --- a/sdk/trace/simple_span_processor.go +++ b/sdk/trace/simple_span_processor.go @@ -90,18 +90,18 @@ func (ssp *simpleSpanProcessor) Shutdown(ctx context.Context) error { select { case err = <-done: - // It is possible for the exporter to have immediately shut down and - // the context to already be done. In that case this select statement - // will randomly choose a case. This could result in a different - // response for similar scenarios. Instead, double check if the - // context is done here and return that error if the exporter shut - // down did not return one. - if err == nil { - // If ctx is not done yet, ctx.Err returns nil. + case <-ctx.Done(): + // It is possible for the exporter to have immediately shut down + // and the context to already be done. In that case this select + // statement will randomly choose a case. This could result in a + // different response for similar scenarios. Instead, double check + // if the exporter shut down at the same time and return that + // error if so. + if expErr, ok := <-done; ok { + err = expErr + } else { err = ctx.Err() } - case <-ctx.Done(): - err = ctx.Err() } }) return err diff --git a/sdk/trace/simple_span_processor_test.go b/sdk/trace/simple_span_processor_test.go index 688cc4703bd..4ac7124c5fc 100644 --- a/sdk/trace/simple_span_processor_test.go +++ b/sdk/trace/simple_span_processor_test.go @@ -40,9 +40,15 @@ func (t *testExporter) ExportSpans(ctx context.Context, spans []sdktrace.ReadOnl return nil } -func (t *testExporter) Shutdown(context.Context) error { +func (t *testExporter) Shutdown(ctx context.Context) error { t.shutdown = true - return nil + select { + case <-ctx.Done(): + // Ensure context deadline tests receive the expected error. + return ctx.Err() + default: + return nil + } } var _ sdktrace.SpanExporter = (*testExporter)(nil) From 41dc7fde0f6f164a9e1787e2f695f0802e2bc33c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 18 Oct 2021 10:33:34 -0700 Subject: [PATCH 4/5] Update changelog description --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cc0f50a208..9a514cb9dfa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed -- The simple span processor shutdown method returns any context error if there is one and the exporter successfully shuts down. (#2290, #2289) +- The simple span processor shutdown method deterministically returns the exporter error status if it simultaneously finishes when the deadline is reached. (#2290, #2289) ## [1.0.1] - 2021-10-01 From 34c8d644d2a163d43cc4efc986007755f5e0aa7d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 18 Oct 2021 11:53:10 -0700 Subject: [PATCH 5/5] Fix deadlock bug --- sdk/trace/simple_span_processor.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/sdk/trace/simple_span_processor.go b/sdk/trace/simple_span_processor.go index 5efe50a9bff..ecc45bc6c38 100644 --- a/sdk/trace/simple_span_processor.go +++ b/sdk/trace/simple_span_processor.go @@ -88,18 +88,22 @@ func (ssp *simpleSpanProcessor) Shutdown(ctx context.Context) error { go shutdown() + // Wait for the exporter to shut down or the deadline to expire. select { case err = <-done: case <-ctx.Done(): // It is possible for the exporter to have immediately shut down - // and the context to already be done. In that case this select - // statement will randomly choose a case. This could result in a - // different response for similar scenarios. Instead, double check - // if the exporter shut down at the same time and return that - // error if so. - if expErr, ok := <-done; ok { - err = expErr - } else { + // and the context to be done simultaneously. In that case this + // outer select statement will randomly choose a case. This will + // result in a different returned error for similar scenarios. + // Instead, double check if the exporter shut down at the same + // time and return that error if so. This will ensure consistency + // as well as ensure the caller knows the exporter shut down + // successfully (they can already determine if the deadline is + // expired given they passed the context). + select { + case err = <-done: + default: err = ctx.Err() } }