You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4net-dev@logging.apache.org by Jo...@rohde-schwarz.com on 2016/08/22 12:44:24 UTC
PluginMap survives LoggerRepositorySkeleton.ResetConfiguration()
Hi,
LoggerRepositorySkeleton.ResetConfiguration() does not clear its plugin
map, while all other structures (render map, level map) are cleared. Is
there a special reason for that or is it a bug? Note that the plugins are
shutdown in LoggerRepositorySkeleton.Shutdown, though.
Currently I have to remove the plugins manually, to "Reset the
repositories configuration to a default state" (quote from the API docs of
`ResetConfiguration` ).
The attached patch changes the logger repository skeleton to also reset
the plugin map at `RestConfiguration`. The question is: Is this the right
way to do, or are there strong reasons for the plugins to stay? I could
not find anything in the revision log; this code basically didn't change
since the initial commit in january 2004.
Regards,
Jonas
Re: PluginMap survives
LoggerRepositorySkeleton.ResetConfiguration()
Posted by Jo...@rohde-schwarz.com.
Dominik Psenner <dp...@apache.org> wrote on 23.08.2016 13:48:13:
> Von: Dominik Psenner <dp...@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 <bo...@apache.org> wrote on 23.08.2016 06:05:23:
>
> > Von: Stefan Bodewig <bo...@apache.org>
> > An: "Log4Net Developers List" <lo...@logging.apache.org>
> > Datum: 23.08.2016 06:06
> > Betreff: Re: PluginMap survives
> LoggerRepositorySkeleton.ResetConfiguration()
> >
> > On 2016-08-22, <Jo...@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
Re: PluginMap survives LoggerRepositorySkeleton.ResetConfiguration()
Posted by Dominik Psenner <dp...@apache.org>.
--------------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.
On 2016-08-23 10:48, Jonas.Baehr@rohde-schwarz.com wrote:
> Stefan Bodewig <bo...@apache.org> wrote on 23.08.2016 06:05:23:
>
> > Von: Stefan Bodewig <bo...@apache.org>
> > An: "Log4Net Developers List" <lo...@logging.apache.org>
> > Datum: 23.08.2016 06:06
> > Betreff: Re: PluginMap survives
> LoggerRepositorySkeleton.ResetConfiguration()
> >
> > On 2016-08-22, <Jo...@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
Re: PluginMap survives
LoggerRepositorySkeleton.ResetConfiguration()
Posted by Jo...@rohde-schwarz.com.
Stefan Bodewig <bo...@apache.org> wrote on 23.08.2016 06:05:23:
> Von: Stefan Bodewig <bo...@apache.org>
> An: "Log4Net Developers List" <lo...@logging.apache.org>
> Datum: 23.08.2016 06:06
> Betreff: Re: PluginMap survives
LoggerRepositorySkeleton.ResetConfiguration()
>
> On 2016-08-22, <Jo...@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
Re: PluginMap survives LoggerRepositorySkeleton.ResetConfiguration()
Posted by Stefan Bodewig <bo...@apache.org>.
On 2016-08-22, <Jo...@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 :-)
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.
Stefan
Re: PluginMap survives LoggerRepositorySkeleton.ResetConfiguration()
Posted by Dominik Psenner <dp...@gmail.com>.
Hard to tell. Worst case is that we implicitly break code that relies on
plugins to stay where they are. It seems logical to me that
ResetConfiguration() should reset all the configuration. Now one could
argue that plugins are not part of the configuration, but rather something
that provides functionality and gets configured. So ResetConfiguration
logically should not purge plugins, but should purge the configuration of
these plugins.
2016-08-22 14:44 GMT+02:00 <Jo...@rohde-schwarz.com>:
> Hi,
>
> LoggerRepositorySkeleton.ResetConfiguration() does not clear its plugin
> map, while all other structures (render map, level map) are cleared. Is
> there a special reason for that or is it a bug? Note that the plugins are
> shutdown in LoggerRepositorySkeleton.Shutdown, though.
> Currently I have to remove the plugins manually, to "Reset the
> repositories configuration to a default state" (quote from the API docs of
> `ResetConfiguration` ).
>
> The attached patch changes the logger repository skeleton to also reset
> the plugin map at `RestConfiguration`. The question is: Is this the right
> way to do, or are there strong reasons for the plugins to stay? I could not
> find anything in the revision log; this code basically didn't change since
> the initial commit in january 2004.
>
> Regards,
> Jonas
>
>
--
Dominik Psenner