fix: prevent throttling deadlock in cloudwatchlogs output#2190
Open
lorespiz wants to merge 1 commit into
Open
Conversation
On a ThrottlingException for CreateLogStream/CreateLogGroup, log-stream creation could wedge for the entire output plugin with nothing logged. LogThrottleRetryer.ShouldRetry performed an unconditional blocking send to a capacity-1 throttle-event channel drained by a single consumer goroutine. The shared TargetManager was bound (via sync.Once) to the first destination's client and retryer; when that destination stopped (e.g. log rotation with auto_removal), its retryer's consumer goroutine exited. The next throttle then filled the buffer and blocked the send forever, and because TargetManager.InitTarget holds a mutex across the create call, all other targets waiting on that mutex stalled as well. Two changes, either of which breaks the deadlock; both applied: - logthrottle.go: make the throttle-event send non-blocking (select with a default) so a full channel or stopped consumer can never block ShouldRetry. The retry decision is unchanged. - cloudwatchlogs.go: give the shared TargetManager its own dedicated retryer and client owned by the plugin, stopped only in Close(), instead of reusing the first destination's client. This decouples the TargetManager lifecycle from any single destination. Tests: - TestShouldRetryDoesNotBlockAfterStop: ShouldRetry returns after the retryer is stopped (fails via timeout on the pre-fix code). - TestSharedRetryerLifecycle: stopping the first destination does not prevent creating additional destinations. - TestInitTargetNoDeadlockUnderThrottling: end-to-end regression driving the real SDK retry loop against an always-throttling endpoint through InitTarget with a stopped retryer consumer; deadlocks on the pre-fix code, passes with the fix.
lorespiz
added
the
ready for testing
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description of the issue
On a ThrottlingException for CreateLogStream/CreateLogGroup, log-stream creation could wedge for the entire output plugin with nothing logged.
LogThrottleRetryer.ShouldRetry performed an unconditional blocking send to a capacity-1 throttle-event channel drained by a single consumer goroutine. The shared TargetManager was bound (via sync.Once) to the first destination's client and retryer; when that destination stopped (e.g. log rotation with auto_removal), its retryer's consumer goroutine exited. The next throttle then filled the buffer and blocked the send forever, and because TargetManager.InitTarget holds a mutex across the create call, all other targets waiting on that mutex stalled as well.
Description of changes
Two changes, either of which breaks the deadlock; both applied:
logthrottle.go: make the throttle-event send non-blocking (select with a default) so a full channel or stopped consumer can never block ShouldRetry. The retry decision is unchanged.cloudwatchlogs.go: give the shared TargetManager its own dedicated retryer and client owned by the plugin, stopped only in Close(), instead of reusing the first destination's client. This decouples the TargetManager lifecycle from any single destination.License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
TestShouldRetryDoesNotBlockAfterStop: ShouldRetry returns after the retryer is stopped (fails via timeout on the pre-fix code).TestSharedRetryerLifecycle: stopping the first destination does not prevent creating additional destinations.estInitTargetNoDeadlockUnderThrottling: end-to-end regression driving the real SDK retry loop against an always-throttling endpoint through InitTarget with a stopped retryer consumer; deadlocks on the pre-fix code, passes with the fix.Manually validated agent behaviour before and after the fix using a local HTTP server mocking
CreateLogGroup/CreateLogStreamthrottled requestsRequirements
Before commiting your code, please do the following steps.
make fmtandmake fmt-shmake lintIntegration Tests
To run integration tests against this PR, add the
ready for testinglabel.