logging-log4net-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jonas.Ba...@rohde-schwarz.com
Subject Re: PluginMap survives LoggerRepositorySkeleton.ResetConfiguration()
Date Wed, 24 Aug 2016 08:57:09 GMT
Dominik Psenner <dpsenner@apache.org> wrote on 23.08.2016 13:48:13:

> Von: Dominik Psenner <dpsenner@apache.org>
> An: log4net-dev@logging.apache.org
> Datum: 23.08.2016 13:49
> Betreff: Re: PluginMap survives 
LoggerRepositorySkeleton.ResetConfiguration()
> 
> --------------8<-----------8<----------- 
> someRepo.Shutdown(); 
> someRepo.ResetConfiguration(); 
> foreach (var plugin in someRepo.Plugins.AllPlugins) 
>         someRepo.Plugins.Remove(plugin); 
> reconfigure... 
> --------------8<-----------8<-----------
> could also read as:
> --------------8<-----------8<----------- 
> someRepo.Shutdown(); 
> someRepo.ResetConfiguration(); 
> someRepo.RemovePlugins(); 
> reconfigure... 
> --------------8<-----------8<-----------
> where RemovePlugins() could be an extension method without changing 
> the official API.

I've changed the proposed patch now so that it just adds a `Clear` method 
to the class `PluginMap` and adds a note the the API docs of `
LoggerRepositorySkeleton.ResetConfiguration`.

This way the original behaviour of the repository is kept for backward 
compatibility but the pitfall of the dangling plugins is documented.

Regards,
Jonas




> On 2016-08-23 10:48, Jonas.Baehr@rohde-schwarz.com wrote:
> Stefan Bodewig <bodewig@apache.org> wrote on 23.08.2016 06:05:23:
> 
> > Von: Stefan Bodewig <bodewig@apache.org> 
> > An: "Log4Net Developers List" <log4net-dev@logging.apache.org> 
> > Datum: 23.08.2016 06:06 
> > Betreff: Re: PluginMap survives 
> LoggerRepositorySkeleton.ResetConfiguration() 
> > 
> > On 2016-08-22, <Jonas.Baehr@rohde-schwarz.com> wrote:
> > 
> > > The question is: Is this the right way to do, or are there strong
> > > reasons for the plugins to stay?
> > 
> > TBH I have no idea.
> > 
> > It might be the case that some people out there rely on this
> > behavior. One indicator for this might be that you are the first 
person
> > after twelve years who wants to change it :-) 
> 
> Well, `someRepository.Plugins.Add(somePlugin)` silently replaces a 
> potentially existing one (including shutdown), so when I just reset 
> the configuration and add all my plugins again, I won't notice the 
> missing plugin reset at first. 
> I also assume that log4net plugins are not that common. 
> 
> 
> > If this causes problems for your use case I'd recommend adding another
> > reset method that also removes the plugins - and clarify the
> > docs-strings for either method. 
> 
> I'm against cluttering the API with several, nearly identical, 
> methods. In my eyes, this will cause more confusion then benefit. 
> 
> I'm using the methods in question in this way: 
> --------------8<-----------8<----------- 
> someRepo.Shutdown(); 
> someRepo.ResetConfiguration(); 
> foreach (var plugin in someRepo.Plugins.AllPlugins) 
>         someRepo.Plugins.Remove(plugin); 
> reconfigure... 
> --------------8<-----------8<----------- 
> 
> The problem is not so much the extra step of removing the plugins; 
> its just annoying. The point is that `Shutdown()` is required to 
> safely close all appenders (I'm using `Hirarchy` as repository). But
> the base implementation of `Shutdown` also shuts down all the 
> plugins. Which is right, but now the plugins are not usable but 
> still present after `ResetConfiguration`. I think this is a pitfall 
> that should be cleaned up. 
> 
> I can imagine two use cases: 
> - Shutdown before quitting the application 
> - Shutdown before reconfigure (therefore: reset configuration) 
> I consider Resetting without a prior shutdown an error. I see no 
> reason to keep shutdown plugins in the repo after reset. 
> Also note that Hirarchy shuts down as part of reset, while the 
> skeleton does not. But that is a problem of virtual methods calling 
> each other and another story. 
> 
> 
> Regards, 
> Jonas 
Mime
View raw message