splunk / splunk-library-dotnetlogging

Support for logging from .NET Tracing and ETW / Semantic Logging ApplicationBlock to Splunk.
http://dev.splunk.com/view/splunk-loglib-dotnet/SP-CAAAEX4
Apache License 2.0
25 stars 38 forks source link

HttpEventCollectorSender FlushAsync task is not started and thus cannot be awaited unless manually started #51

Open seanmarthur opened 5 years ago

seanmarthur commented 5 years ago

I believe the FlushAsync method is implemented incorrectly:

https://github.com/splunk/splunk-library-dotnetlogging/blob/master/src/Splunk.Logging.Common/HttpEventCollectorSender.cs#L292-L298

There are examples on the web (see here) showing the usage as:

await ecSender.FlushAsync();

However, when trying to run such a sample, the application hangs forever at this line. This is because the underling task never starts. The proper usage as the code currently is would be:

var flushTask = ecSender.FlushAsync();
flushTask.Start();
await flushTask;

However the consumer should not need to start this task manually - it should be returned from FlushAsync in a started state.

Refactored FlushAsync example:

    public Task FlushAsync()
    {
      var task = new Task(() =>
     {
       FlushSync();
     });
      task.Start();
      return task;
    }
shakeelmohamed commented 5 years ago

Hi @seanmarthur,

We're open to a pull request to address this issue if you're interested. Thanks for the note

bluedog13 commented 3 years ago

"This is because the underling task never starts"

I've seen the POST commands happening behind the scenes, but the control never returns back to the method calling the "FlushAsync()".

@seanmarthur - what you have observed is a very valid observation and is indeed an issue that I too have faced.

It may also help if the return type has an object with some information about the completed work and not just "Task" itself. We have no insight to what success really means unless we capture the exception and infer the results.