search

serilog-contrib / serilog-sinks-splunk

A Serilog sink that writes to Splunk
https://splunk.com
Apache License 2.0
46 stars 47 forks source link

Possible thread leak when ILogger instances are disposed. #63

Closed CoderNumber1 closed 7 years ago

CoderNumber1 commented 7 years ago

I have a web application which spins up and disposes of logs as needed while it's running. After adding in the Splunk sink I noticed the application running slower with a much higher CPU utilization. It looked like the application's thread count would just grow constantly. Looking at the EventCollectorSink constructor I notice it calls RepeatAction.OnInterval(TimeSpan pollInterval, Action action, CancellationToken token) but doesn't cancel the returned task on dispose. My guess is then each time an ILogger is spun up a new task/thread is created and it continues to attempt to run after the ILogger is disposed until the web app recycles. I wonder if the returned task could be held in a private field by the sink and cancelled on dispose or maybe even let that sink inherit from the Periodic Batch Sink and let that deal with batching log messages.

https://github.com/serilog/serilog-sinks-splunk/blob/092d929ea92624f98970844e46e91e7878db54b4/src/Serilog.Sinks.Splunk/Sinks/Splunk/EventCollectorSink.cs#L193

https://github.com/serilog/serilog-sinks-splunk/blob/092d929ea92624f98970844e46e91e7878db54b4/src/Serilog.Sinks.Splunk/Sinks/Splunk/RepeatAction.cs#L26

merbla commented 7 years ago

Hey @CoderNumber1, Yes the repeat action was around a bit before the periodic sink. We have chatted about moving the tried and tested period batching sink.

I think is it a good enhancement.

CoderNumber1 commented 7 years ago

@merbla Would you mind if I forked the repo and took a shot an resolving the issue myself? I would enjoy the opportunity to submit a pull request.

merbla commented 7 years ago

@CoderNumber1 absolutely go for it!! 👍

CoderNumber1 commented 7 years ago

@merbla Sorry for the radio silence, I've had to put this down in favor of some other projects recently so I haven't been able to devote much time to it. I've been doing some testing today though that looks promising. I'm running both the original and periodic batching versions side by side in separate executables trying to re-create a thread leak in each. The former is up over 3000 threads so far while the updated version has been holding steady at 3 for the last 30 minutes or so. I'll try to submit a pull request for this in the next few days.

merbla commented 7 years ago

👍 great work!