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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
