Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Fix dispatch group leave in LogstashDestinationSocket #129

Merged

Conversation

natanrolnik
Copy link
Contributor

LogstashDestinationSocket never calls the complete block in the LogstashDestinationSocket.sendLogs(_:transform:queue:complete:) function. After debugging, I found the issue was related to never leaving the DispatchGroup 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 also LogstashDestinationWriter, and therefore LogstashDestination) never calls the completion block. This PR fixes it by correctly managing the DispatchGroup that caused the bug.

Motivation and Context

After investigating, I noticed the following issue in the lines below:

    let dispatchGroup = DispatchGroup()
    for log in logs.sorted(by: { $0.0 < $1.0 }) {
        // get the log
        dispatchGroup.enter()
        task.write(logData, timeout: self.timeout) { [weak self] error in
        guard let self = self else {
            dispatchGroup.leave()
            return
        }

    self.dispatchQueue.async(group: dispatchGroup) {
        if let error = error {
            sendStatus[tag] = error
        }
    }

According to the docs, leave() needs to have balanced calls with enter(). Although enter is called, leave() is only called in the guard branch, and the regular flow continues to using the dispatch queue. However, here's the thing. Associating a dispatch group with the DispatchQueue.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 calling leave() in the end.

How Has This Been Tested?

Although there are tests in LogstashDestinationTests, they use MockLogstashDestinationSocket, so they don't cover this issue. There would need to be tests specific to LogstashDestinationSocket.

I tested the completion block behavior before and after the changes, confirming the issue and the fix.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@albertodebortoli
Copy link
Member

The workflow failed when validating the pod

error: Cannot code sign because the target does not have an Info.plist file and one is not being generated automatically. Apply an Info.plist file to the target using the INFOPLIST_FILE build setting or generate one automatically by setting the GENERATE_INFOPLIST_FILE build setting to YES (recommended). (in target 'App' from project 'App')

[!] JustLog did not pass validation, due to 1 error.

This is not crucial as the author is interested in consuming it via SPM.

The repo either needs some maintenance (and, for example, the drop of CocoaPods) or a deprecation notice anyway.

@albertodebortoli albertodebortoli merged commit 5a29497 into justeat:master Jan 2, 2024
1 check failed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants