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
ManagerBase.toString()
- OK.
FileStore.directory()
- This may be targeted at testing environments. Who knows how one may
try using that component?
> 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);
}
}
]]]
> 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.
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.
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".
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)
(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.)
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().
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".
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
