Dominik Psenner <dpsen...@apache.org> wrote on 23.08.2016 13:48:13: > Von: Dominik Psenner <dpsen...@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.ba...@rohde-schwarz.com wrote: > Stefan Bodewig <bode...@apache.org> wrote on 23.08.2016 06:05:23: > > > Von: Stefan Bodewig <bode...@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.ba...@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
PluginMap-Clear_v2.patch
Description: Binary data
