> On May 8, 2020, at 12:29 PM, Alan Carroll <[email protected]> > wrote: > > Consider a situation with option (1) with two remap rules: > > map http://example.one <http://example.one/> http://example.one > <http://example.one/> @plugin=txn_box.so @reloadable=false blah blah blah > map http://example.two <http://example.two/> http://example.two > <http://example.two/> @plugin=txn_box.so @reloadable=true blah blah > > Does that DSO get reloaded on a reload of "remap.config"?
It should get reloaded for the second, not for the first. As far as I understand, this is fine, the Gancho made the code such that as long as something uses some version of a plugin, it will be kept forever. — Leif > > > On Fri, May 8, 2020 at 9:58 AM Sudheer Vinukonda <[email protected] > <mailto:[email protected]>> wrote: > Ah, true. I get the misunderstanding now. Yeah, I don’t mean to have > reloadable flexibility per remap line either, but just per “(remap)plugin”. > > And the only point I was trying to make was to let that the flexibility be > determined by the user and not implicitly by the fact that a plugin was used > in mixed mode. And yeah sorry, I totally missed the problem with making it a > remap level param instead of a plugin level param. So, I still prefer your > approach 1, except it’d be clearer if it’s named something more obvious > indicating non-reload ability than “@global” (but, naming is hard and I can’t > think of a short/succinct better name :() > > > >> On May 8, 2020, at 7:33 AM, Alan Carroll <[email protected] >> <mailto:[email protected]>> wrote: >> >> >> Sudheer, I understand the point you are making, I just consider it >> irrelevant. Let me give Leif an example to illustrate why - TxnBox. It >> shares data between the global and remap configurations at run time via >> static variables. If you enable remap DSO reloading for TxnBox, it will >> crash on the first transaction that hits a remap rule. It doesn't matter if >> it's actually been reloaded or not. However your organization does plugin >> updates, TxnBox will still crash in that situation. Even in your example, >> Sudheer, there's no _choice_ about whether a particular plugin can be DSO >> reloaded, it's a result of the implementation. As you yourself write, you >> can't enable it for those plugins without changing the code. No >> configuration cleverness will get around that. >> >> For plugins that do support DSO reloading, the enablement is still per >> plugin, not per remap rule. Moreover, if we went with option (3) it would be >> simple to have to plugin support a configuration / load time option to >> enable or disable DSO reloading. In general, if the plugin can be DSO >> reloaded, it's unclear why it shouldn't be except in unusual circumstances >> which are depending on the plugin implementation. >> >> For Sudheer, I remain unclear on what exact flexibility you want, given the >> constraints created by a specific plugin's implementation. I've re-read your >> note and AFAICT it assumes doing DSO reload or not *per plugin*, which is >> also my point. I dislike (1) because it makes no sense to me to have this >> change between remap rules for a specific plugin. I think it's better to >> have the plugin decide if that's possible and, if needed, provide >> configuration to disable it if needed. Speaking specifically for TxnBox, I >> must forbid you from enabling DSO reloading. Even in your case, it might be >> reasonable to have this for plugins that you have not yet updated (which is >> actually the case with TxnBox - I'm limited by a requirement for ATS 7 >> compatibility, so I can't change that feature at the current time). >> >> On Thu, May 7, 2020 at 10:11 PM Leif Hedstrom <[email protected] >> <mailto:[email protected]>> wrote: >> >> >>> On May 7, 2020, at 8:12 PM, Alan Carroll <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> Leif; >>> >>> If the plugin can be global or remap but not both, I don't see why (2) >>> limits anything. The entire issue is irrelevant for such plugins, because >>> the situation of reloading the remap DSO but not the global cannot occur, >>> In fact, option (3) or (4) would enable detecting this and issuing a >>> warning. >> >> Ah yes, good point. However, still the same problem, one can very much want >> to use say header_rewrite as both global and remap plugin at the same time, >> and be fine with the fact that it doesn’t reload as a “global”, but you want >> it to reload as a remap. We use that plugin in this way for example. >> >> I still feel that option 2) is a bad option, but I’m ok with the others >> (still with a preference towards #1). I think a finer granular control >> mechanism here is a good idea. >> >> I’d also be curious to hear which of the core plugins are having problems >> here, in most cases, there’s a no dependency between the global >> instantiation, and the per remap instantiation. Sudheer and LinkedIn have >> many internal plugins that do experience this problem however, so I’m >> guessing that maybe you have similar custom internal plugins? >> >> Cheers, >> >> — Leif >> >>> >>> Approach (1) was my first thought, but I think the problem there is whether >>> the plugin can work as a global and a reloadable remap is a property of the >>> plugin implementation, not any particular remap rule. That is, for a >>> specific plugin, there's really no choice about whether to use "@plugin" or >>> "@global" - the configuration must get it right or the plugin crashes. >>> Every time. Every rule. It is for this reason I disagree with Leif's view >>> the user should decide. The user's opinion is irrelevant - the plugin works >>> in this mode, or it doesn't. And as our friends at LinkedIn discovered, >>> some rather basic C++ decisions (such as using static variables) will >>> prevent a plugin from working in this mode. On the other hand, if the >>> plugin uses the "User Args" feature then it can work, in which case what's >>> the point of disabling the DSO reload? Unless the plugin implementor is >>> concerned about code skew between the global and remap versions, which >>> again the user is not qualified to decide. >>> >>> My personal preference is (3), but I suspect after mysterious crashes with >>> plugins, we will have been happier with (4). >>> >>> On Thu, May 7, 2020 at 7:42 PM Sudheer Vinukonda >>> <[email protected] <mailto:[email protected]>> wrote: >>> +1 on the general idea to make the reloadability customizable per plugin. >>> >>> However, I think it'd be more simple, cleaner and intuitive to not tie it >>> to whether or not a plugin is used both as a global and remap plugin. >>> >>> In other words, approach (1) below but, instead of calling it "@global", we >>> could add a param which says "@reloadable=false" (the default value for >>> "@reloadable" can be "true"). >>> >>> The same param can then be used, when we eventually add relodability to >>> global plugins as well. >>> >>> Thoughts? >>> >>> >>> >>> >>> On Thursday, May 7, 2020, 05:24:09 PM PDT, Alan Carroll >>> <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> >>> As part of the ATS 9 upgrade, a feature was added so that remap plugins >>> could have their DSO reloaded. This means not just the configuration, but >>> the implementation itself. While very useful, this has some unfortunate >>> side effects with plugins that are used in both a global and remap context. >>> To alleviate this, a configuration variable as added to disable the feature. >>> >>> Although reasonable, this is a rather heavy handed way to deal with the >>> problem. What would be better is the ability to reload the DSO or not on a >>> per remap plugin basis. I have a few ways this could be done: >>> >>> 1) Add the keyword "@global" to "remap.config". This would behave exactly >>> as "@plugin" except it would prohibit reloading of the DSO for that plugin. >>> >>> 2) Have the remap reload configuration check to see if the plugin is also a >>> global plugin and disable remap DSO reload for that plugin. >>> >>> 3) Add a flag to the global plugin registry information which can be set >>> during TSPluginInit which disables DSO reloading for that plugin, should it >>> occur in "remap.config". This is similar to (2) but requires a plugin to >>> prohibit DSO reloading. The call woud be TSPluginDSOReloadEnable(flag) and >>> would only be valid when called from TSPluginInit. >>> >>> 4) As (3), except the flag is set by default and must be cleared to enable >>> DSO reloading in "remap.config". >>> >>> I'm willing to see if I can make this work, but I would like to have some >>> feedback on the preferred approach first. >>> >>
