Skip to content

Commit

Permalink
Fix shutdown test cleanup
Browse files Browse the repository at this point in the history
Do not check the second call to the client Stop. There is no guarantee
it will not error in normal operation.
  • Loading branch information
MrAlias committed Nov 22, 2021
1 parent ab98555 commit 7067fe0
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 15 deletions.
22 changes: 12 additions & 10 deletions exporters/otlp/otlptrace/internal/otlptracetest/client.go
Expand Up @@ -56,11 +56,12 @@ func initializeExporter(t *testing.T, client otlptrace.Client) *otlptrace.Export

func testClientStopHonorsTimeout(t *testing.T, client otlptrace.Client) {
t.Cleanup(func() {
// The test is looking for a failed shut down. Make sure the client is
// actually closed at the end thought to clean up any used resources.
if err := client.Stop(context.Background()); err != nil {
t.Fatalf("failed to stop client: %v", err)
}
// The test is looking for a failed shut down. Call Stop a second time
// with an un-expired context to give the client a second chance at
// cleaning up. There is not guarantee from the Client interface this
// will succeed, therefore, no need to check the error (just give it a
// best try).
_ = client.Stop(context.Background())
})
e := initializeExporter(t, client)

Expand All @@ -75,11 +76,12 @@ func testClientStopHonorsTimeout(t *testing.T, client otlptrace.Client) {

func testClientStopHonorsCancel(t *testing.T, client otlptrace.Client) {
t.Cleanup(func() {
// The test is looking for a failed shut down. Make sure the client is
// actually closed at the end thought to clean up any used resources.
if err := client.Stop(context.Background()); err != nil {
t.Fatalf("failed to stop client: %v", err)
}
// The test is looking for a failed shut down. Call Stop a second time
// with an un-expired context to give the client a second chance at
// cleaning up. There is not guarantee from the Client interface this
// will succeed, therefore, no need to check the error (just give it a
// best try).
_ = client.Stop(context.Background())
})
e := initializeExporter(t, client)

Expand Down
22 changes: 18 additions & 4 deletions exporters/otlp/otlptrace/otlptracegrpc/client.go
Expand Up @@ -109,9 +109,23 @@ func (c *client) Start(ctx context.Context) error {
return nil
}

var errNotStarted = errors.New("client not started")
var errAlreadyStopped = errors.New("the client is already stopped")

// Stop shuts down the gRPC connection to the collector.
// Stop shuts down the client.
//
// Any active connections to a remote endpoint are closed if they were created
// by the client. Any gRPC connection passed during creation using
// WithGRPCConn will not be closed. It is the caller's responsibility to
// handle cleanup of that resource.
//
// This method synchronizes with the UploadTraces method of the client. It
// will wait for any active calls to that method to complete unimpeded, or it
// will cancel any active calls if ctx expires. If ctx expires, the context
// error will be forwarded as the returned error. All client held resources
// will still be released still in this situation.
//
// If the client has already stopped, an error will be returned describing
// this.
func (c *client) Stop(ctx context.Context) error {
// Acquire the c.tscMu lock within the ctx lifetime.
acquired := make(chan struct{})
Expand Down Expand Up @@ -143,7 +157,7 @@ func (c *client) Stop(ctx context.Context) error {
// client is started before doing anything and let the called know if they
// made a mistake.
if c.tsc == nil {
return errNotStarted
return errAlreadyStopped
}

// Clear c.tsc to signal the client is stopped.
Expand All @@ -159,7 +173,7 @@ func (c *client) Stop(ctx context.Context) error {
return err
}

var errShutdown = errors.New("exporter is shutdown")
var errShutdown = errors.New("the client is shutdown")

// UploadTraces sends a batch of spans.
//
Expand Down
2 changes: 1 addition & 1 deletion exporters/otlp/otlptrace/otlptracegrpc/client_unit_test.go
Expand Up @@ -125,7 +125,7 @@ func TestRetryable(t *testing.T) {

func TestUnstartedStop(t *testing.T) {
client := NewClient()
assert.ErrorIs(t, client.Stop(context.Background()), errNotStarted)
assert.ErrorIs(t, client.Stop(context.Background()), errAlreadyStopped)
}

func TestUnstartedUploadTrace(t *testing.T) {
Expand Down

0 comments on commit 7067fe0

Please sign in to comment.