[prev in list] [next in list] [prev in thread] [next in thread]
List: log4net-dev
Subject: Re: PluginMap survives LoggerRepositorySkeleton.ResetConfiguration()
From: Jonas.Baehr () rohde-schwarz ! com
Date: 2016-08-24 8:57:09
Message-ID: OF23BC8644.54224F6A-ONC1258019.003018F1-C1258019.00312D98 () rohde-schwarz ! com
[Download RAW message or body]
--=_mixed 00312D4EC1258019_=
Content-Type: multipart/alternative; boundary="=_alternative 00312D4EC1258019_="
--=_alternative 00312D4EC1258019_=
Content-Type: text/plain; charset="US-ASCII"
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
--=_alternative 00312D4EC1258019_=
Content-Type: text/html; charset="US-ASCII"
<tt><font size=2>Dominik Psenner <dpsenner@apache.org> wrote on
23.08.2016 13:48:13:<br>
<br>
> Von: Dominik Psenner <dpsenner@apache.org></font></tt>
<br><tt><font size=2>> An: log4net-dev@logging.apache.org</font></tt>
<br><tt><font size=2>> Datum: 23.08.2016 13:49</font></tt>
<br><tt><font size=2>> Betreff: Re: PluginMap survives \
LoggerRepositorySkeleton.ResetConfiguration()</font></tt> <br><tt><font size=2>> \
<br> > --------------8<-----------8<----------- <br>
> someRepo.Shutdown(); <br>
> someRepo.ResetConfiguration(); <br>
> foreach (var plugin in someRepo.Plugins.AllPlugins) <br>
> someRepo.Plugins.Remove(plugin); <br>
> reconfigure... <br>
> --------------8<-----------8<-----------</font></tt>
<br><tt><font size=2>> could also read as:</font></tt>
<br><tt><font size=2>> --------------8<-----------8<-----------
<br>
> someRepo.Shutdown(); <br>
> someRepo.ResetConfiguration(); <br>
> someRepo.RemovePlugins(); <br>
> reconfigure... <br>
> --------------8<-----------8<-----------</font></tt>
<br><tt><font size=2>> where RemovePlugins() could be an extension method
without changing <br>
> the official API.</font></tt>
<br>
<br><tt><font size=2>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`.</font></tt>
<br>
<br><tt><font size=2>This way the original behaviour of the repository
is kept for backward compatibility but the pitfall of the dangling plugins
is documented.</font></tt>
<br>
<br><tt><font size=2>Regards,</font></tt>
<br><tt><font size=2>Jonas</font></tt>
<br>
<br>
<br>
<br>
<br><tt><font size=2>> On 2016-08-23 10:48, Jonas.Baehr@rohde-schwarz.com
wrote:</font></tt>
<br><tt><font size=2>> Stefan Bodewig <bodewig@apache.org> wrote
on 23.08.2016 06:05:23:<br>
> <br>
> > Von: Stefan Bodewig <bodewig@apache.org> <br>
> > An: "Log4Net Developers List" \
<log4net-dev@logging.apache.org> <br>
> > Datum: 23.08.2016 06:06 <br>
> > Betreff: Re: PluginMap survives <br>
> LoggerRepositorySkeleton.ResetConfiguration() <br>
> > <br>
> > On 2016-08-22, <Jonas.Baehr@rohde-schwarz.com> wrote:<br>
> > <br>
> > > The question is: Is this the right way to do, or are there
strong<br>
> > > reasons for the plugins to stay?<br>
> > <br>
> > TBH I have no idea.<br>
> > <br>
> > It might be the case that some people out there rely on this<br>
> > behavior. One indicator for this might be that you are the first
person<br>
> > after twelve years who wants to change it :-) <br>
> <br>
> Well, `someRepository.Plugins.Add(somePlugin)` silently replaces a
<br>
> potentially existing one (including shutdown), so when I just reset
<br>
> the configuration and add all my plugins again, I won't notice the
<br>
> missing plugin reset at first. <br>
> I also assume that log4net plugins are not that common. <br>
> <br>
> <br>
> > If this causes problems for your use case I'd recommend adding
another<br>
> > reset method that also removes the plugins - and clarify the<br>
> > docs-strings for either method. <br>
> <br>
> I'm against cluttering the API with several, nearly identical, <br>
> methods. In my eyes, this will cause more confusion then benefit.
<br>
> <br>
> I'm using the methods in question in this way: <br>
> --------------8<-----------8<----------- <br>
> someRepo.Shutdown(); <br>
> someRepo.ResetConfiguration(); <br>
> foreach (var plugin in someRepo.Plugins.AllPlugins) <br>
> someRepo.Plugins.Remove(plugin); <br>
> reconfigure... <br>
> --------------8<-----------8<----------- <br>
> <br>
> The problem is not so much the extra step of removing the plugins;
<br>
> its just annoying. The point is that `Shutdown()` is required to <br>
> safely close all appenders (I'm using `Hirarchy` as repository). But<br>
> the base implementation of `Shutdown` also shuts down all the <br>
> plugins. Which is right, but now the plugins are not usable but <br>
> still present after `ResetConfiguration`. I think this is a pitfall
<br>
> that should be cleaned up. <br>
> <br>
> I can imagine two use cases: <br>
> - Shutdown before quitting the application <br>
> - Shutdown before reconfigure (therefore: reset configuration) <br>
> I consider Resetting without a prior shutdown an error. I see no <br>
> reason to keep shutdown plugins in the repo after reset. <br>
> Also note that Hirarchy shuts down as part of reset, while the <br>
> skeleton does not. But that is a problem of virtual methods calling
<br>
> each other and another story. <br>
> <br>
> <br>
> Regards, <br>
> Jonas </font></tt>
--=_alternative 00312D4EC1258019_=--
--=_mixed 00312D4EC1258019_=
Content-Type: application/octet-stream; name="PluginMap-Clear_v2.patch"
Content-Disposition: attachment; filename="PluginMap-Clear_v2.patch"
Content-Transfer-Encoding: base64
SW5kZXg6IFBsdWdpbi9QbHVnaW5NYXAuY3MNCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBQbHVnaW4vUGx1Z2lu
TWFwLmNzCShyZXZpc2lvbiAxNzU2NTE0KQ0KKysrIFBsdWdpbi9QbHVnaW5NYXAuY3MJKHdvcmtp
bmcgY29weSkNCkBAIC0xNjMsNiArMTYzLDcgQEANCiAJCS8vLyA8cmVtYXJrcz4KIAkJLy8vIDxw
YXJhPgogCQkvLy8gUmVtb3ZlIGEgc3BlY2lmaWMgcGx1Z2luIGZyb20gdGhpcyBtYXAuCisJCS8v
LyBOb3RlIHRoYXQgdGhlIHBsdWdpbiBpcyBub3Qgc2h1dCBkb3duIQogCQkvLy8gPC9wYXJhPgog
CQkvLy8gPC9yZW1hcmtzPgogCQlwdWJsaWMgdm9pZCBSZW1vdmUoSVBsdWdpbiBwbHVnaW4pCkBA
IC0xNzcsNiArMTc4LDIxIEBADQogCQkJfQogCQl9CiAKKyAgICAgICAgLy8vIDxzdW1tYXJ5Pgor
ICAgICAgICAvLy8gQ2xlYXJzIHRoZSBtYXAuCisgICAgICAgIC8vLyA8L3N1bW1hcnk+CisgICAg
ICAgIC8vLyA8cGFyYT4KKyAgICAgICAgLy8vIFJlbW92ZSBhbGwgcGx1Z2lucyBmcm9tIHRoaXMg
bWFwLgorICAgICAgICAvLy8gTm90ZSB0aGF0IHRoZSBwbHVnaW5zIGFyZSBub3Qgc2h1dCBkb3du
IQorICAgICAgICAvLy8gPC9wYXJhPgorICAgICAgICBwdWJsaWMgdm9pZCBDbGVhcigpCisgICAg
ICAgIHsKKyAgICAgICAgICAgIGxvY2sgKHRoaXMpCisgICAgICAgICAgICB7CisgICAgICAgICAg
ICAgICAgbV9tYXBOYW1lMlBsdWdpbi5DbGVhcigpOworICAgICAgICAgICAgfQorICAgICAgICB9
CisKIAkJI2VuZHJlZ2lvbiBQdWJsaWMgSW5zdGFuY2UgTWV0aG9kcwogCiAJCSNyZWdpb24gUHJp
dmF0ZSBJbnN0YW5jZSBGaWVsZHMKSW5kZXg6IFJlcG9zaXRvcnkvTG9nZ2VyUmVwb3NpdG9yeVNr
ZWxldG9uLmNzDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09DQotLS0gUmVwb3NpdG9yeS9Mb2dnZXJSZXBvc2l0b3J5U2tl
bGV0b24uY3MJKHJldmlzaW9uIDE3NTY1MTQpDQorKysgUmVwb3NpdG9yeS9Mb2dnZXJSZXBvc2l0
b3J5U2tlbGV0b24uY3MJKHdvcmtpbmcgY29weSkNCkBAIC0yNzcsNiArMjc3LDExIEBADQogCQkv
Ly8gPHBhcmE+CiAJCS8vLyBFeGlzdGluZyBsb2dnZXJzIGFyZSBub3QgcmVtb3ZlZC4gVGhleSBh
cmUganVzdCByZXNldC4KIAkJLy8vIDwvcGFyYT4KKyAgICAgICAgLy8vIDxwYXJhPgorICAgICAg
ICAvLy8gQXR0YWNoZWQgcGx1Z2lucyBhcmUgbm90IHJlbW92ZWQuCisgICAgICAgIC8vLyBJZiB5
b3UndmUgPHNlZSBjcmVmPSJTaHV0ZG93biIvPiB0aGUgcmVwb3NpdG9yeSB0aGUgcGx1Z2lucyBh
cmUgc2h1dGRvd24sIHRvby4KKyAgICAgICAgLy8vIEluIHRoYXQgY2FzZSB5b3UgbmVlZCB0byBj
YWxsIDxzZWUgY3JlZj0iTTpQbHVnaW5NYXAuQ2xlYXIiLz4gb24gPHNlZSBjcmVmPSJNOlBsdWdp
bk1hcCIvPiB0byBmdWxseSByZXNldCB0aGUgY29uZmlndXJhdGlvbi4KKyAgICAgICAgLy8vIDwv
cGFyYT4KIAkJLy8vIDxwYXJhPgogCQkvLy8gVGhpcyBtZXRob2Qgc2hvdWxkIGJlIHVzZWQgc3Bh
cmluZ2x5IGFuZCB3aXRoIGNhcmUgYXMgaXQgd2lsbAogCQkvLy8gYmxvY2sgYWxsIGxvZ2dpbmcg
dW50aWwgaXQgaXMgY29tcGxldGVkLgo=
--=_mixed 00312D4EC1258019_=--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic