This repository has been archived by the owner on Jan 5, 2024. It is now read-only.
Fix dispatch group leave in LogstashDestinationSocket #129
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.
LogstashDestinationSocket
never calls thecomplete
block in theLogstashDestinationSocket.sendLogs(_:transform:queue:complete:)
function. After debugging, I found the issue was related to never leaving theDispatchGroup
it uses to manage the completion.Description
While debugging logs not displaying in our server end (which was related to something else), I found that
LogstashDestinationSocket
(and therefore alsoLogstashDestinationWriter
, and thereforeLogstashDestination
) never calls the completion block. This PR fixes it by correctly managing theDispatchGroup
that caused the bug.Motivation and Context
After investigating, I noticed the following issue in the lines below:
According to the docs,
leave()
needs to have balanced calls withenter()
. Although enter is called,leave()
is only called in theguard
branch, and the regular flow continues to using the dispatch queue. However, here's the thing. Associating a dispatch group with theDispatchQueue.async(group:qos:flags:execute:)
method, takes care of both entering and leaving a group automatically. So, in this case, the group receives 2 enter calls (one manual, one automatic), and only one leave call (automatic).You can confirm this behavior in this SwiftFiddle.
There are a number of ways to fix this, but I found the simplest one is omitting the group in the
async
call, and manually callingleave()
in the end.How Has This Been Tested?
Although there are tests in
LogstashDestinationTests
, they useMockLogstashDestinationSocket
, so they don't cover this issue. There would need to be tests specific toLogstashDestinationSocket
.I tested the completion block behavior before and after the changes, confirming the issue and the fix.
Types of changes
Checklist: