You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Mark Thomas <ma...@apache.org> on 2016/01/08 13:33:46 UTC
[PROPOSAL] Manager behaviour clarification for 9.0.x
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.
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.
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.
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).
Option 1 is fairly simple to implement as large parts of the code
essentially assume this already and all the current Tomcat code already
conforms to this requirement. This option would require documenting the
requirement, adding a few lines of code to enforce it and then removing
various null checks that would no longer be necessary.
Option 2 would require a lot more work, particularly 2a. 2a would create
a lot of strange edge cases (e.g. Context changes mid-way through
sessions being persisted to the store - where do those sessions get
written?) which would require careful definition and implementation. 2b
would be manageable but we'd need to carefully define "the Manager not
being used" and which methods could only be called in that state.
My preference is for 1. It is less work, Tomcat currently never changes
the Context and I can't see a viable use case for wanting to do so.
Assuming there are no objections, I'll add this to my TODO list.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PROPOSAL] Manager behaviour clarification for 9.0.x
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 1/8/16 7:33 AM, Mark Thomas wrote:
> 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.
>
> 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.
>
> 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.
>
> 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).
>
>
> Option 1 is fairly simple to implement as large parts of the code
> essentially assume this already and all the current Tomcat code already
> conforms to this requirement. This option would require documenting the
> requirement, adding a few lines of code to enforce it and then removing
> various null checks that would no longer be necessary.
>
> Option 2 would require a lot more work, particularly 2a. 2a would create
> a lot of strange edge cases (e.g. Context changes mid-way through
> sessions being persisted to the store - where do those sessions get
> written?) which would require careful definition and implementation. 2b
> would be manageable but we'd need to carefully define "the Manager not
> being used" and which methods could only be called in that state.
>
> My preference is for 1. It is less work, Tomcat currently never changes
> the Context and I can't see a viable use case for wanting to do so.
>
> Assuming there are no objections, I'll add this to my TODO list.
+1 for the easy option.
If someone wants to play games with context-switching, they ought to be
able to use the parallel-deployment features to achieve the same goal
without all the danger involved in lobotomizing a running context.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PROPOSAL] Manager behaviour clarification for 9.0.x
Posted by Rémy Maucherat <re...@apache.org>.
2016-01-08 13:33 GMT+01:00 Mark Thomas <ma...@apache.org>:
> My preference is for 1. It is less work, Tomcat currently never changes
> the Context and I can't see a viable use case for wanting to do so.
>
No problem, that's the best solution.
Rémy
Re: [PROPOSAL] Manager behaviour clarification for 9.0.x
Posted by Konstantin Kolinko <kn...@gmail.com>.
2016-01-08 20:26 GMT+03:00 Mark Thomas <ma...@apache.org>:
> 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
I guess I was looking at the current implementation after your fix.
OK, those are unusable outside of a web application.
Or it makes sense to use them offline?
- to read session data from a standalone application,
- in a test
> Assumes non-null:
> ManagerBase
> getObjectNameKeyProperties()
> getDomainInternal()
> SingleSignOn
> SingleSignOnSessionKey
OK.
> Checks for null
> setContext()
> toString()
The null checks are needed in both these places. The toString() can be
called at any time.
> SingleSingOnEvent
I think you mean SingleSignOnListener.sessionEvent().
A sanity check. Not really needed. But that method is full of sanity
checks, so one more does not hurt.
>> [...]
>> 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.
>
>
> 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.
OK.
A call to context.removePropertyChangeListener() is needed .
Instead of setContext(null) call it will be more clear to add the
listener in initInternal() and to remove it in destroyInternal().
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PROPOSAL] Manager behaviour clarification for 9.0.x
Posted by Mark Thomas <ma...@apache.org>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PROPOSAL] Manager behaviour clarification for 9.0.x
Posted by Konstantin Kolinko <kn...@gmail.com>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org