logging-log4net-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dominik Psenner <dpsen...@apache.org>
Subject Re: AsyncAppenderSkeleton
Date Mon, 31 Oct 2016 14:30:35 GMT
See inlines.

On 2016-10-31 11:30, Joe wrote:
>
> Hi Dominik,
>
> Thanks for the feedback
>
> > Please note also that MSMQ sounds like a MS only service and that would in turn
mean that the .net core 
> variant would no longer be able to benefit from the 
> AsyncAppenderSkeleton. To me this outlines a path that we would not 
> like to walk on
>
> I don’t see the problem here.
>
> I’m proposing that we could implement the queue as a separate class 
> implementing a suitable interface (ILoggingEventQueue, IAppenderQueue 
> or whatever – I’m with Philip Karlton on naming).    The default 
> implementation would be an in-memory queue, would not depend on MSMQ 
> and would be available for .net core etc.
>

Sorry, my fault, the sentence was TL;DR it's entirety. I had it read it 
as "The default implementation could be MSMQ". ;-) Thanks for the 
clarification.

> Then there could be an alternate implementation with a persistent 
> queue using MSMQ, or users could provide their own custom 
> implementations using some other persistent queueing technology if 
> they wish.
>
> The alternative of a persistent queue is useful to reduce the risk of 
> (probably the last and therefore most important) logging events being 
> lost when an application crashes: with a persistent queue they could 
> be picked up again from the queue when the application restarts, or by 
> an independent utility.
>
> > This sounds mostly like the BufferingAppenderSkeleton, which only 
> misses the background worker thread to send the buffer.
>
> I’m not convinced that BufferingAppenderSkeleton is a good candidate. 
>  For example:
>
> - Locking is problematic.  The appender would need to lock(this) while 
> it is forwarding logging events to the sink 
> (BufferingAppenderSkeleton.SendBuffer).  This could hold the lock for 
> an extended period (e.g. due to a communication timeout).  Therefore 
> DoAppend should not lock(this) while enqueueing logging events or 
> we’ll be unnecessarily blocking the calling application.  This is one 
> of the main reasons I want to implement my own DoEvents rather than 
> deriving from AppenderSkeleton.
>

If the implementation requires lock(this) to work, the implementation is 
broken. The queue itself has to be thread safe. Hence, a true async 
appender should block the calling application only to fix a few logging 
event properties that would otherwise be lost (i.e. stacktrace or thread 
information).

> - I see the buffering and triggering logic being implemented in a 
> pluggable ILoggingEventQueue.   By default, there would be no 
> buffering, except what is implicit in the fact that events may be 
> enqueued faster than they can be dequeued.  I.e. whenever the 
> background thread detects events in the queue, by default it processes 
> all available events immediately, in blocks whose maximum size is a 
> configurable SendBufferSize.    A custom queue implementation could 
> implement triggering logic akin to BufferingAppenderSkeleton, e.g. 
> wait for a timeout defined by TimeEvaluator if there are fewer than 
> SendBufferSize events in the queue.
>

A async appender working in async mode always buffers, by definition. If 
it wouldn't buffer, there would be nothing that a background thread 
could work on and it would block the calling application.

> > System.Threading.Task.Run().
>
> The TPL could be one way of implementing the queue, though I’m not 
> convinced that it adds much value.  The custom implementation I did 
> recently didn’t use TPL, and that would be my starting point.  This 
> also means it would be compatible with .NET 3.5.
>

If .net 3.5 is a target for async logging, then the implementation 
cannot use the System.Threading.Tasks namespace. Otherwise I would build 
upon the default task scheduler implementation or provide a custom task 
scheduler implementation that derives from 
System.Threading.Tasks.TaskScheduler and let all logging tasks run on 
that task scheduler.

>   I found having a single background thread made it easier to 
> implement things like flushing.
>

Mileage may vary but to me, this is not the case.

>    Flush was implemented to:
>
> - return true immediately if the queue is empty and all events have 
> been successfully sent to the sink.
>
> - return false immediately if the last attempt to send events to the 
> sink failed.
>
> - else wait for the background thread to set a ManualResetEvent when 
> it’s finished processing all events in the queue.
>

That could read as:

bool Flush() {
return await Task.Run(() => {
   return doFlush();
});
}

or:

bool Flush() {
  Task<bool> task = Task.Run() => {
   return doFlush();
});
  task.Wait();
  return task.Result;
}

or even:

Task<bool> FlushAsync() {
return Task.Run() => {
   return doFlush();
});
}

> > The default implementation should probably be able to operate 
> asynchronously or synchronously and change mode of operation based on 
> a configuration flag "Asynchronous=True"
>
> That’s exactly what I had in mind.
>

Cheers

Mime
View raw message