ParallelForwardingAppender do not set CurrentThread.Name in TPL

[xmedeko]

I think maybe better would be not to use TPL, but start own background thread for the SubscriberLoop() . Because the logger may block the thread for a longer time, so the TPL is not appropriate for this. Then, setting the thread name will be ok.

[cjbhaines]

Fair point. @rcollette any comments on this?

[rcollette]

I don't know that I can agree with this. For example. An appender that makes a network request to do its writing SHOULD use async/await or in some other appropriate way implement non-blocking i/o and that shouldn't affect the TPL.

If that's the case then using TPL SHOULD allow the most efficient use of threads on the machine.

If not, then I would suggest creating a new Appender type that is a different implementation. The ParallelForwardingAppender is by definition an appender that relies on the TPL.

Like Mr Oracle says. A test is worth a thousand words.

[xmedeko]

log4net appenders are not async by default. I use log4net on a client, Desktop app and cannot assume any HW conditions, like network speed or even disk speed. (E.g. when a disk has bad sectors, then a single write may have a substantial delay.) So, for me ParallelForwardingAppender is a protection to keep our app running even when logging freezes. So, I've made my own version of ParallelForwardingAppender with a background Thread instead of TPL. Also, note that even some adapters, like AdoNetAppender may communicate via network and are not async.

[rcollette]

I'd call it ThreadedForwardingAppender or something similar as a new appender implementation type and add it to the project. This way you are not changing a known behavior in the library and users can choose which implementation to use and when.

[xmedeko]

May be the solution, too. But what's the use case for the ParallelForwardingAppender then?

[rcollette]

The use case is that the TPL properly manages thread workloads on a machine given that there are a finite number of threads that can be executed at any given point in time by the underlying hardware. Creating a new thread because you don't want to block another thread doesn't take into account what the system is actually capable of doing at any point in time.

The other "use case" for preserving it is to give people time and opportunity to convert to the new one and test. This library had other forwarding appender implementation options that existed simultaneously. I favor deprecation periods rather than suddenly replacing one behavior with another.

While I would say that a blocking appender shouldn't be used because it is the root cause of the problem, there is the reality that you may be forced to deal with a non-behaving appender, and if that is the case, it should be your choice to use a forwarding appender that addresses that scenario. Create a DataFlowForwardingAppender and give people the opportunity to try it. Run some performance tests and compare the two to provide evidence of which is better. If there is a clearly better one, then get rid of the old one after a deprecation period.

To date, I have not had a production issue with either file or database writing with the TPL based appender where it was imposing some wait state elsewhere in my application. If I had a working example/test of where a behaving (i.e. non-blocking) appender is being used with the parallel forwarding appender and the parallel forwarding appender was impacting the client application, then I would of course say fix-it or chuck it. Quite the opposite, with heavy logging loads (usually in my internal environments only) I've seen it improve application performance substantially vs. not using it at all (I'm not going to provide any figures that aren't based on a legitimate test scenario). So I have no base in experience from which I can agree that there is "no use case" for it.

A thought debate like this is irrelevant. What's really required for decision making is side by side test data.

[xmedeko]

I've looked more closely into the code ParallelForwardingAppender and if I understand it right it makes one long running task blocking one thread form the TPL pool for the application lifetime. Then the behaviour is the same like creating own thread as I have proposed. But the TPL tasks should be short running, they should not block the thread for more then a few hundred ms.

If you need a long running task use TaskCreationOptions.LongRunning. It creates a new thread in the current .NET implementation. But even using LongRunning thread you should not set the thread name, because the .NET implementation may change in the future. So, I think it's better to start a new, custom thread and set the thread name.

[cjbhaines]

I've looked into the BlockCollection internals and I agree with Richard here. The thread is blocked for the duration of the appender lifetime, therefore the thread pool won't have access to the thread. This is not a problem though because it is not spinning, it just means there is one less thread in the pool. If you are struggling with thread exhaustion in your pool then look at using ThreadPool.SetMinThreads but we are talking about a single thread here, it should expand.

[xmedeko]

Do at least use TaskCreationOptions.LongRunning.

Note: also do not create a Task and call Start() unless you find an extremely good reason to do so, see https://stackoverflow.com/questions/29693362/.

[rcollette]

The use of LongRunning and StartNew I agree with. @xmedeko - Good bit of information you just provided.

StartNew isn't going to have a huge impact here because it's only called once for the application, but good practice is good practice.