Fix Flaky Semaphore Test #391
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elastic-observability-automation
bot
added
the
safe-to-test
1pkg
reviewed
Nov 5, 2024
|
|
||
| var once sync.Once |
There was a problem hiding this comment.
I think, we don't really need this complicated concurrency primitive structure for signalling here and we can simplify the test. The same test can be accomplished with a single buffered channel without any additional primitives, see the example below.
func TestConsumeLogsSemaphore(t *testing.T) {
logs := plog.NewLogs()
var batches []*modelpb.Batch
doneCh := make(chan struct{})
recorder := modelpb.ProcessBatchFunc(func(ctx context.Context, batch *modelpb.Batch) error {
doneCh <- struct{}{}
doneCh <- struct{}{}
batchCopy := batch.Clone()
batches = append(batches, &batchCopy)
return nil
})
consumer := otlp.NewConsumer(otlp.ConsumerConfig{
Processor: recorder,
Semaphore: semaphore.NewWeighted(1),
})
go func() {
_, err := consumer.ConsumeLogsWithResult(context.Background(), logs)
assert.NoError(t, err)
}()
<-doneCh
ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond)
defer cancel()
_, err := consumer.ConsumeLogsWithResult(ctx, logs)
assert.Equal(t, err.Error(), "context deadline exceeded")
<-doneCh
// turn channel into sink
doneCh = make(chan struct{}, 2)
_, err = consumer.ConsumeLogsWithResult(context.Background(), logs)
assert.NoError(t, err)
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #361
Context
Testing
apm-datawith-racecauses a deadlock, since the semaphore is not properly released.The TestConsumeLogsSemaphore basically consists of 3 parts:
Solution
Removing the following lines from function semAcquire seems to fix the issue.
Initially, I thought the problem was propagating an inappropriate context, but passing a background context and removing the context shadowing, didn't solve the problem.
The only reasonable explanation, therefore, is the span introduces enough of a lag such that Goroutine 1 does not acquire the semaphore first, before Main starts competing for it, leading to a deadlock.
The deadlock is solved by "upgrading" the startCh to sync the parallel running goroutine with main, not when it starts, but when it has fully acquired the semaphore. This is the purpose of
semAcquiredCh.However, since the processor is executed multiple times, for the context of this test we only want to sync the first acquisition. Hence, the use of
sync.Once.Test