[exporterhelper] Fix potential deadlocks in BatcherSender shutdown #10258
Conversation
dmitryax
force-pushed
the
fix-batcher-shutdown-deadlock
branch
from
769744f to
56f36e9
Compare
dmitryax
force-pushed
the
fix-batcher-shutdown-deadlock
branch
from
56f36e9 to
4ab3a33
Compare
|
I'm guessing I missed something since |
dmitryax
force-pushed
the
fix-batcher-shutdown-deadlock
branch
from
4ab3a33 to
25d9fb7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10258 +/- ##
=======================================
Coverage 92.46% 92.46%
=======================================
Files 387 387
Lines 18264 18269 +5
=======================================
+ Hits 16888 16893 +5
Misses 1029 1029
Partials 347 347 ☔ View full report in Codecov by Sentry. |
dmitryax
force-pushed
the
fix-batcher-shutdown-deadlock
branch
from
25d9fb7 to
0491e66
Compare
|
The actual regression test was still failing. I updated the code, should be good now. |
| for i := 0; i < 10; i++ { | ||
| startWG.Add(1) | ||
| go func() { | ||
| startWG.Done() |
There was a problem hiding this comment.
| startWG.Done() | |
| defer startWG.Done() |
This should be after the be.send call, right? Maybe use defer instead? Or are you purposefully wanting to continue in the main goroutine before send is complete?
There was a problem hiding this comment.
No, it should be before send. We need to ensure all the goroutines are started. The send is blocking
There was a problem hiding this comment.
We need to ensure all the goroutines are started. The send is blocking
The goroutines will all be started regardless of where the startWG.Done() call is made, if I'm following correctly. The only impact is when the main goroutine will continue after the startWG.Wait() call. I'm wondering if it should wait until all sends are complete or not?
There was a problem hiding this comment.
the send operations cannot complete until they are unblocked, which we do after the shutdown - that's the purpose of the test case
There was a problem hiding this comment.
Okay, thanks for clarifying 👍
|
|
||
| // 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()) |
There was a problem hiding this comment.
Is it valid to send two batches in some cases, as your comment is referencing? If so, should the assert here be 2 instead of 1?
There was a problem hiding this comment.
It's usually 1 or more in rare cases. LessOrEqual checks that the first argument is less or equal to the second one
Fixes #10255