On 08/01/2016 16:56, Konstantin Kolinko wrote:
> 2016-01-08 15:33 GMT+03:00 Mark Thomas <[email protected]>:
>> 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: [email protected]
For additional commands, e-mail: [email protected]
