cjbhaines / Log4Net.Async

Asynchronous Log4Net appenders and forwarder
http://www.nuget.org/packages/Log4Net.Async/
MIT License
121 stars 37 forks source link

Created ParallelForwardingAppender, use volatile modifier on shutDownRequested #4

Closed rcollette closed 9 years ago

rcollette commented 9 years ago

Created a base class AsyncForwardingAppenderBase.

Made AsyncForwardingAppender inherit from AsyncFowardingBase and removed members that are defined in the base class.

Created class ParallelForwardingAppender, inheriting from AsyncForwardingAppenderBase. This class uses BlockingCollection to implement a lossless async forwarding appender that does not have to sleep in the consumer thread loop. Created associated unit tests.

Added the volatile modifier to the shutDownRequested member because it is accessed from two different threads and we want to ensure that a cached version of the value is not being read in the subscriber thread loop.

Verified all tests pass.

rcollette commented 9 years ago

Regarding the tests. Many of the issues were not code issues but timing tolerances in the tests. Timings or number of items that process are going to vary between physical instances. One of your tests failed by 1 millisecond.

In this commit, I updated tests to reflect more general conditions such as "some number of entries must be appended before appender close and some number of entries must process after appender close when the buffer is mostly full at the time of close." Please have a good look over of the test changes if you don't mind.

I added some thread cleanup to avoid exceptions that were happening on the test platform's appdomain unload. Just now I'm recalling that Tasks should be disposed, which I do in the ParallelForwardingAppender but not in the tests. It's late (early really) here and it doesn't affect test operation but should be a shore up task.

I noticed the test for Identity being empty. It isn't under VSTest and VS 2013 and so that test was failing on my home machine which is VS2013. I commented out that test and placed a note in there referring to an article that explains policies for copying the Identity to new threads.

I changed the ParallelForwardingAppender to use ConsumingEnumerable, which is a cleaner loop than using a while/take loop. Take throws a rather generic InvalidOperationException when the collection is completed. ConsumingEnumerable handles that gracefully without throwing an exception. OperationCanceledException is now handled. Previously, though unit tests were passing, "background" exceptions were happening on task completion/cancellation. I didn't realize this at first because expected internal exceptions caused intentionally by some of the tests appear in the console inline with exceptions that shouldn't have been there. You should notice fewer messages in the console when running the tests as compared with the last commit.

This is my first time seeing AppVeyer running live. It's very cool. All tests passed, but again, the tests themselves should be reviewed.

rcollette commented 9 years ago

Just looked at the diff and noticed that disposal of the appender needs to check that the task has been completed or canceled, handle appropriately if not, and then dispose the task. Hold off on merging.

rcollette commented 9 years ago

I think this is good to merge now assuming you agree with the test changes.

I am a little perplexed by the fact that all tests ran successfully on my machine at home and here the database tests are failing. (VS2013 vs VS2012 issue?). It's a little bit unusual to see actual database connections in a unit test.

cjbhaines commented 9 years ago

Hi Richard, Great job on this it looks like a much better solution. I just have one last question on the thread recovery in the catch block of Exception in the SubscriberLoop. Sorry for the slow turn around on this I have been quite busy at work this week. I will try and push any updates tomorrow while at work and as soon as we are green I'll push the NuGet package update. Cheers,Chris

Date: Wed, 29 Oct 2014 07:51:41 -0700 From: notifications@github.com To: Log4Net.Async@noreply.github.com Subject: Re: [Log4Net.Async] Created ParallelForwardingAppender, use volatile modifier on shutDownRequested (#4)

I think this is good to merge now assuming you agree with the test changes.

I am a little perplexed by the fact that all tests ran successfully on my machine at home and here the database tests are failing. (VS2013 vs VS2012 issue?). It's a little bit unusual to see actual database connections in a unit test.

— Reply to this email directly or view it on GitHub. =

rcollette commented 9 years ago

Per my line comments, this has been updated to complete the subscriber loop on unknown exceptions occurring in the BlockingCollection enumerator.