tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: [PROPOSAL] Manager behaviour clarification for 9.0.x
Date Fri, 08 Jan 2016 18:08:57 GMT
2016-01-08 20:26 GMT+03:00 Mark Thomas <markt@apache.org>:
> On 08/01/2016 16:56, Konstantin Kolinko wrote:
>> 2016-01-08 15:33 GMT+03:00 Mark Thomas <markt@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


Mime
View raw message