> On May 7, 2020, at 8:12 PM, Alan Carroll <[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. >
