Review of connection and model handling

[simoneb]

Hey gimmi,

unless I'm missing something about how log4net works I believe that the way you handle the model is not completely right, as the RMQ .NET client implements the model in a way that is not thread-safe, therefore you should not share the model among different threads.
I'm not suggesting you create a new model every time you have to log something either - although models are lightweight in the .NET implementation it is still not advisable to create too many of them on a single connection. Instead I'd suggest you set up a producer-consumer model using perhaps the thread-safe collections in .NET and on the producer side just enqueue data from the logger while using a single model on the consumer side to publish that to RMQ.

For more details about RMQ .NET client behavior I suggest you look at the API documentation, it's very useful.

[gimmi]

Hi Simone,

Log4Net custome appender should be used in a thread safe manner by Log4Net. Evidences of this are in AppenderSkeleton abstract class (the class Log4Rabbit appender inherit from):

https://github.com/apache/log4net/blob/trunk/src/Appender/AppenderSkeleton.cs#L297

By looking at the code of AppenderSkeleton, i noted that overriding this method:

https://github.com/apache/log4net/blob/trunk/src/Appender/AppenderSkeleton.cs#L631

exposes to potential concurrency. Overriding this method is needed to enable "batch" logging and so it is crucial for performances. I will further investigate this.

thanks for reviewing the code.

[simoneb]

No worries, that's pretty much what I meant by missing something about how log4net works. I wouldn't expect it to be thread-safe, but of course in case it turns out to be safe then your code is just fine.

Besides threading issues, however, I would also wrap the calls to model and connection into try/catch blocks, because some race condition may happen there as well. This is explained in the .NET API docs too.

[gimmi]

By design Log4Net should never cause a logging exception to propagate in calling application. This:

https://github.com/apache/log4net/blob/trunk/src/Appender/AppenderSkeleton.cs#L320

and this:

https://github.com/apache/log4net/blob/trunk/src/Appender/AppenderSkeleton.cs#L427

should be enough. Do you think that this:

https://github.com/gimmi/Log4Rabbit/blob/master/src/Log4Rabbit/ModelHandler.cs#L18

is enough to detect invalid connection/channel?

[simoneb]

Strictly speaking it's not, because a channel/connection might get closed between the time you check and when you then use it (think about a network connection being shut down for external reasons). I concur it's not an issue in most cases, as it's unlikely to happen, but you at least need to be aware of it.
If log4net itself does not propagate the exception to the client then this is even less of a problem, but still, if you don't want to lose messages the suggested way to do it is to try, catch the exception and retry.

[gimmi]

I've reworked connection handling. The emphasis has been put on ensuring that the calling application will not be blocked, even at the cost of loosing log messages.

[simoneb]

I see, I believe you ended up doing something very similar to what I did in Roger in fact (ReliableConnection class), so that should be good :)

[gimmi]

Sounds interesting, i'll take a look at it :)