On 08/01/2016 16:56, Konstantin Kolinko wrote: > 2016-01-08 15:33 GMT+03:00 Mark Thomas <ma...@apache.org>: >> I've been looking at the relationship between a Context and its Manager >> and I have identified some inconsistencies in the underlying assumptions >> on which the code is based. >> >> A. Context is always set >> In some places the code carefully checks to see if the Context is null. >> In other places (often in the same method and before the null check) the >> code assumes that the Context is non-null. > > Specific examples = ? I see only a few checks
null check vs no null check: [File|JDBC]Store before r1723736 Assumes non-null: ManagerBase getObjectNameKeyProperties() getDomainInternal() SingleSignOn SingleSignOnSessionKey Checks for null setContext() toString() SingleSingOnEvent Just to start with. >> B. Context never changes >> ManagerBase.setContext() is written on the basis that the Context may >> change at any point. However, pretty much everywhere else assumes that >> the Context doesn't change (at least while the Manager is in use). >> >> Related to B, there are various places where threading issues exist if >> the Context were to change while the Manager was in use and a few places >> (e.g. the distributable flag) that aren't updated that should be. > > Good catch about "distributable", but I think it is just a bug. > Compare it with the following other property: > > ManagerBase.setContext() gets the current value of sessionTimeout and > registers itself as PropertyChangeListener. > > ManagerBase.propertyChange() handles update of "sessionTimeout". > > The "sessionTimeout" property of StandardContext can be changed via JMX. > > Technically, the "distributable" property is exposed to JMX as well > and is changeable, though I do not see much use in changing it. > > I think it is better to notify "distributable" field change via > PropertyListener. Currently StandardContext.setDistributable() has a > workaround: > > [[[ > @Override > public void setDistributable(boolean distributable) { > boolean oldDistributable = this.distributable; > this.distributable = distributable; > support.firePropertyChange("distributable", > oldDistributable, > this.distributable); > > // Bugzilla 32866 > if(getManager() != null) { > if(log.isDebugEnabled()) { > log.debug("Propagating distributable=" + distributable > + " to manager"); > } > getManager().setDistributable(distributable); > } > } > ]]] I think the problem is wider than distributable. Lots of places assume the Context is always non-null. >> Given these issues I see two options. >> >> 1. Require that the Context is set before the Manager is first used and >> disallow any changes once the Manager has been used. >> > > Make it dependent on Lifecycle.getState(), > or you are just saying about (oldValue != null) check? > >> 2. Correctly handle the possibility that the Context may change either >> a) at any time or b) if the Context is stopped (i.e. the Manager is not >> being used). > > First review > ========== > I think it should be possible to replace Manager in a Context before > that Context starts. No problem with that. > The default implementation can be wrong and one should be able to > replace it. That is usually performed by a LifecycleListener. > > If so, Context shall call setContext(null) on the old manager instance > so that the old Manager unregisters itself as PropertyChangeListener > on the context. > > Actually currently there is no such setContext(null) call, which is a bug. We can fix that. > So I think it is 2.b) - allow change when it is stopped. > The Manager is a Lifecycle, so you can say "if Manager is stopped". I'm not entirely against replacing the Manager in a running Context although the consequences are likely to be messy. But I wasn't coming at this from that direction. I was coming at this from changing the Context for a Manager. I don't think we should / need to support this. Once a Manager has been used with a Context it should be thrown away once it is no longer required. > Replacing a component after startup may be useful, but I think for a > manager this feature is not needed. A Manager is a heavy component > (StandardManager loads serialized sessions at startup), so I think it > does not need this feature. > > As such, Context.setManager() can be simplified. > > (If some 3rd party one really needs to replace manager on a running > context, the caller of Context.setManager() can call the lifecycle > methods on manager by itself and deal with consequences) I'm not sure we need to support this but at this point I'm not looking to explicitly block this. > (It is odd that current Context.setManager() calls oldManager.stop() > newManager.start(), as there is a time interval when there is no > usable manager. It would be better to call newManager.start() > oldManager.stop(), or call setPaused(true) to stop serving requests.) I'll look at this. I think we also need to call oldManager.destroy() given the Manager is not meant to be re-used. > Second review > =========== > From point of view of LifecycleListener that customizes a Manager > > StandardContext.startInternal() does > > // Notify our interested LifecycleListeners > fireLifecycleEvent(Lifecycle.CONFIGURE_START_EVENT, null); > > before calling getManager() and configuring the default manager, so it > theory it should be possible to configure a Manager at that time. > > The next lifecycle event at end of startInternal() is the following: > > setState(LifecycleState.STARTING); > > is too late to replace a manager, as the "state" field is changed > before notifying lifecycle listeners, and the Context becomes > available to serve requests. > > As such, changing a manager on "START_EVENT" requires support of "2.a) > - allow change at any time". I am OK with not allowing manager change > on that event. > > I think implementing "1. no change" is not possible, because > StandardContext.reload() method calls stop(); start(). That should be fine. The Manager/Context relationship isn't changing. It a user wants to do stop(), setManager(newManager), start() that is fine too. What they won't be able to then to is take the oldManager and use it somewhere else. > One may need to change a manager during "CONFIGURE_START_EVENT" and > the manager can have been already set by previous run. So it needs > "2.b) - allow change when it is stopped". That should be fine. In summary, what I am intending is: Context.manager -> change whenever you like although change while running at your own risk Manager.context -> no changes once the Manager is not in the NEW state. I hope this clarifies what I was proposing. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org