Skip to content

Commit

Permalink
[chore] Remove redundant wait group from a test (open-telemetry#10269)
Browse files Browse the repository at this point in the history
https://github.com/open-telemetry/opentelemetry-collector/pull/10258/files#r1621017272
made me wonder if the wait group is really necessary. We have another
synchronization that waits for at least two requests to enter the merge
function, which should be enough. So, we don't necessarily need this
wait group.

Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
  • Loading branch information
2 people authored and steves-canva committed Jun 13, 2024
1 parent 33acd13 commit bd6c3ba
Showing 1 changed file with 8 additions and 18 deletions.
26 changes: 8 additions & 18 deletions exporter/exporterhelper/batch_sender_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -443,9 +443,10 @@ func TestBatchSender_ShutdownDeadlock(t *testing.T) {
waitMerge := make(chan struct{}, 10)

// blockedBatchMergeFunc blocks until the blockMerge channel is closed
blockedBatchMergeFunc := func(_ context.Context, r1 Request, _ Request) (Request, error) {
blockedBatchMergeFunc := func(_ context.Context, r1 Request, r2 Request) (Request, error) {
waitMerge <- struct{}{}
<-blockMerge
r1.(*fakeRequest).items += r2.(*fakeRequest).items
return r1, nil
}

Expand All @@ -458,18 +459,11 @@ func TestBatchSender_ShutdownDeadlock(t *testing.T) {

sink := newFakeRequestSink()

// Send 10 concurrent requests and wait for them to start
startWG := sync.WaitGroup{}
for i := 0; i < 10; i++ {
startWG.Add(1)
go func() {
startWG.Done()
require.NoError(t, be.send(context.Background(), &fakeRequest{items: 4, sink: sink}))
}()
}
startWG.Wait()
// Send 2 concurrent requests
go func() { require.NoError(t, be.send(context.Background(), &fakeRequest{items: 4, sink: sink})) }()
go func() { require.NoError(t, be.send(context.Background(), &fakeRequest{items: 4, sink: sink})) }()

// Wait for at least one batch to enter the merge function
// Wait for the requests to enter the merge function
<-waitMerge

// Initiate the exporter shutdown, unblock the batch merge function to catch possible deadlocks,
Expand All @@ -485,12 +479,8 @@ func TestBatchSender_ShutdownDeadlock(t *testing.T) {
close(blockMerge)
<-doneShutdown

// The exporter should have sent only one "merged" batch, in some cases it might send two if the shutdown
// happens before the batch is fully merged.
assert.LessOrEqual(t, uint64(1), sink.requestsCount.Load())

// blockedBatchMergeFunc just returns the first request, so the items count should be 4 times the requests count.
assert.Equal(t, sink.requestsCount.Load()*4, sink.itemsCount.Load())
assert.EqualValues(t, 1, sink.requestsCount.Load())
assert.EqualValues(t, 8, sink.itemsCount.Load())
}

func queueBatchExporter(t *testing.T, batchOption Option) *baseExporter {
Expand Down

0 comments on commit bd6c3ba

Please sign in to comment.