You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Rémy Maucherat <re...@apache.org> on 2014/01/14 18:03:53 UTC
Context switch integration
Hi,
In Tomcat, entering a context is done simply by setting the context
classloader and sometimes firing some random events (like the ones from the
ServletRequestListener). This isn't so good for integration and could use
improvements, in addition to at least one place where this is not done at
all (very recently a bug reported to me that indicated there was an issue
with SSO session expiration, and indeed it expires sessions from other
contexts just like that).
So I would propose harmonizing this (and fix the SSO issue too, obviously),
with a new dedicated ThreadBindingListener interface (two methods:
bind/unbind) and a get/set for it also in Context, to be used for
integration. Although usually generic listeners are used (and there can be
multiple ones), this has worked well for me so far.
Ideally I would have liked to also take over the privileged actions in a
utility class, but this would make PA use less optimal (for example in the
request dispatcher).
Comments ? (or better ideas ?)
Rémy
Re: Context switch integration
Posted by Rémy Maucherat <re...@apache.org>.
2014/1/22 Mark Thomas <ma...@apache.org>
> I've been looking at this some more with an eye to reducing code
> duplication. I think the thread binding listener could be merged with
> the ContainerListener. I can't see a good reason to keep them apart at
> this point. With this in mind, the changes I'm planning are:
>
> - Add bindThread() and unbindThread() to the Context interface
> - Add optional PA (as in callers can opt to use it if they wish)
> support to bindThread() and unbindThread()
> - Switch all the components currently implementing their own
> bind/unbind methods to use these new context methods
> - Switch container event type to use an enum
> - Add bind and unbind events to this new enum
> - Remove ThreadBindingListener
>
> The main benefit of this is that all the actions required on bind and
> unbind will be in a single place rather than duplicated (sometimes
> inconsistently) across the code base.
>
> +1
Rémy
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
On 23/01/2014 12:22, Rémy Maucherat wrote:
> 2014/1/23 Mark Thomas <ma...@apache.org>
>
>> http://people.apache.org/~markt/dev/2014-01-23-tccl-tc8-v2.patch
>>
>> It was easier to write, removes more code and the result is easier to
>> read. Unless there are objections I plan to commit this along with some
>> additional work to migrate other code that is currently doing this itself.
>>
>>
> Ok, let's try that.
Done (by accident - I was on the wrong branch when I did a "git svn
dcommit" but no harm done).
> Don't forget to keep SecurityClassLoad in sync.
We have a test case that should catch those sorts of issues now.
The one thing the patch doesn't do is make the calling of the
ThreadBindingListener optional. We could add a parameter for that if
necessary.
We can also remove the DEFAULT_NAMING_LISTENER as all uses of the
listener are from StandardContext and it checks for null.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Rémy Maucherat <re...@apache.org>.
2014/1/23 Mark Thomas <ma...@apache.org>
> http://people.apache.org/~markt/dev/2014-01-23-tccl-tc8-v2.patch
>
> It was easier to write, removes more code and the result is easier to
> read. Unless there are objections I plan to commit this along with some
> additional work to migrate other code that is currently doing this itself.
>
>
Ok, let's try that. Don't forget to keep SecurityClassLoad in sync.
Rémy
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
On 23/01/2014 09:00, Mark Thomas wrote:
> On 22/01/2014 22:03, Rémy Maucherat wrote:
>> 2014/1/22 Mark Thomas <ma...@apache.org>
>
>>> I'll see what folks think of the patch so far and maybe explore the
>>> entry/exit option as well.
>>>
>> The first patch however looks more complicated than the "dumb" way IMO.
>
> The variation of the code that needs to be run (return value / no return
> value, different checked exceptions) adds most of the complication.
>
> I think I'll try, as you put it, a dumb version of the same patch and
> see what it looks like.
http://people.apache.org/~markt/dev/2014-01-23-tccl-tc8-v2.patch
It was easier to write, removes more code and the result is easier to
read. Unless there are objections I plan to commit this along with some
additional work to migrate other code that is currently doing this itself.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
On 22/01/2014 22:03, Rémy Maucherat wrote:
> 2014/1/22 Mark Thomas <ma...@apache.org>
>> I'll see what folks think of the patch so far and maybe explore the
>> entry/exit option as well.
>>
> The first patch however looks more complicated than the "dumb" way IMO.
The variation of the code that needs to be run (return value / no return
value, different checked exceptions) adds most of the complication.
I think I'll try, as you put it, a dumb version of the same patch and
see what it looks like.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Rémy Maucherat <re...@apache.org>.
2014/1/22 Mark Thomas <ma...@apache.org>
> I thought about that. There is some coupling between the entry and
> exit methods (the class loader) that the caller would need to retain
> and some standard try/finally plumbing.
>
> There is also an optimisation not to try to change the class loader if
> there is no need to do so.
>
That looks like a good summary !
>
> In an effort to reduce this common code for every caller, I started
> down a different route. I do wonder if the entry/exit approach might
> end up being cleaner as even with my approach, you end up with some
> not so nice exception handling.
>
> I'll see what folks think of the patch so far and maybe explore the
> entry/exit option as well.
>
> The first patch however looks more complicated than the "dumb" way IMO.
Rémy
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 22/01/2014 20:48, Christopher Schultz wrote:
>> I'll start again and address my primary concern - the duplicate
>> code. The other aspects I'm happy to think about / discuss
>> further with a view to possibly coming back to them later.
>
> Could this be done more simply by providing a pair of utility
> methods to do the enter/exit work rather than treating them as
> events to be "handled"? That way, they can be invoked directly by
> components that need them and ignored by those that don't.
I thought about that. There is some coupling between the entry and
exit methods (the class loader) that the caller would need to retain
and some standard try/finally plumbing.
There is also an optimisation not to try to change the class loader if
there is no need to do so.
In an effort to reduce this common code for every caller, I started
down a different route. I do wonder if the entry/exit approach might
end up being cleaner as even with my approach, you end up with some
not so nice exception handling.
I'll see what folks think of the patch so far and maybe explore the
entry/exit option as well.
Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.18 (Darwin)
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJS4DAnAAoJEBDAHFovYFnneMoP/2oA7IeOH+e7kENRJauAIQT9
2RqHHbfbE54m27PQUk8JtstskokT/p0AoTmrELo9VfJocgWKAjzD39+Zhsm8YgVk
9Z+eW25rheG/YDYTBHxNAMAl79EQhBDzyVL+04Ltgn11PLD9uqZR4HdOdietFXQc
wOXPW/hnnTbPXAXpK+EsXXPU5JDskjUIoXxWw7jjYX+MUHRsbnlbc8r9ky17sX3B
kyjcjLYGCXuB5zFzwVKzFGhA4Kx9vcLYq3Fw/bA1qf64JA9F7BljFnhxaotFVshQ
WIFjNl/g7ZwZV717iPUvUaP+/M1/Mq4EiE5iPRL+k8tGOBMR8A88z4dQ1nPmrsC/
HrJ7mJIaXKoyAj0GGmCcdSG1IlqINQa/zxe2LVZ6p2JsFA7ryNf0vvfzo4OSVqBP
M0A8PcC6kyrHlQ24SyGRfqcMzTiZdHQ0sFS1uYBbESkuyJ9ooxTR2/eCarzemvj6
KIUOqgDJcnHaHI/zjelrC5vBWlWIyZX2+AI27moO34YC5I3KIJ9DhJ99KjumGZup
PdMhszy2nFXHE2s9Q4H7mz5MJWQYmCYF/ecJV3evWgicA1EFVoM0jQfGRwRYTuhe
QdPD8ljAa7VvGZAcXcqfUDjmQ+l9SX/9nNLGIIEKLT+pHNsiL0WRPCGzxFGvOMQA
kppqlPlBvNJdecCKPVPb
=TamI
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 1/22/14, 5:21 AM, Mark Thomas wrote:
> On 22/01/2014 09:42, Konstantin Kolinko wrote:
>> 2014/1/22 Mark Thomas <ma...@apache.org>:
>
> <snip/>
>
>>> I've been looking at this some more with an eye to reducing code
>>> duplication. I think the thread binding listener could be merged with
>>> the ContainerListener. I can't see a good reason to keep them apart at
>>> this point.
>>
>> 1. If there are many ContainerListeners installed you will be calling
>> them with an event that they are not interested to handle. It wastes
>> time.
>
> Fair point. This is something that gets called several times on every
> request. That probably makes the case for a separate listener.
>
>>
>>> With this in mind, the changes I'm planning are:
>>>
>>> - Add bindThread() and unbindThread() to the Context interface
>>
>> Do you mean as a generic replacement to Thread.setContextClassLoader() calls?
>> I think the current methods of StandardContext are not suitable for
>> such exposure.
>
> Sort of, with some refactoring because of the JNDI aspects you
> mentioned. My main aim is to reduce code duplication.
>
>>> - Add optional PA (as in callers can opt to use it if they wish)
>>> support to bindThread() and unbindThread()
>>
>> What do you mean by "PA"?
>
> PrivilegedAction
>
>>> - Switch all the components currently implementing their own
>>> bind/unbind methods to use these new context methods
>>> - Switch container event type to use an enum
>>
>> Enums are better than Strings for performance,
>> but I am not sure whether those can be extended if needed.
>>
>> I'd prefer to keep Strings here.
>
> Do we need to extend these? This is all internal to Tomcat. We can add
> values to the enum as easily as we can use new String values.
>
>>> - Add bind and unbind events to this new enum
>>> - Remove ThreadBindingListener
>>>
>>> The main benefit of this is that all the actions required on bind and
>>> unbind will be in a single place rather than duplicated (sometimes
>>> inconsistently) across the code base.
>
> This is really what I am aiming at. There are several places across the
> code that do call near enough exactly the code to bind and unbind the
> context class loader. It is this duplication I want to remove.
>
> I'll start again and address my primary concern - the duplicate code.
> The other aspects I'm happy to think about / discuss further with a view
> to possibly coming back to them later.
Could this be done more simply by providing a pair of utility methods to
do the enter/exit work rather than treating them as events to be
"handled"? That way, they can be invoked directly by components that
need them and ignored by those that don't.
-chris
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
On 22/01/2014 10:21, Mark Thomas wrote:
> I'll start again and address my primary concern - the duplicate code.
> The other aspects I'm happy to think about / discuss further with a view
> to possibly coming back to them later.
This is the sort of thing I was thinking about:
http://people.apache.org/~markt/dev/2014-01-22-tccl-tc8-v1.patch
It doesn't reduce the code as much as I had hoped.
It isn't complete but you get the idea of how all the class loader
binding/unbinding could move into the Context.
The main benefit is consistency. There is quite a variation in how class
loader binding/unbinding is done at the moment. I think, ultimately, it
will result in slightly less code.
What do folks think?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
On 22/01/2014 09:42, Konstantin Kolinko wrote:
> 2014/1/22 Mark Thomas <ma...@apache.org>:
<snip/>
>> I've been looking at this some more with an eye to reducing code
>> duplication. I think the thread binding listener could be merged with
>> the ContainerListener. I can't see a good reason to keep them apart at
>> this point.
>
> 1. If there are many ContainerListeners installed you will be calling
> them with an event that they are not interested to handle. It wastes
> time.
Fair point. This is something that gets called several times on every
request. That probably makes the case for a separate listener.
>
>> With this in mind, the changes I'm planning are:
>>
>> - Add bindThread() and unbindThread() to the Context interface
>
> Do you mean as a generic replacement to Thread.setContextClassLoader() calls?
> I think the current methods of StandardContext are not suitable for
> such exposure.
Sort of, with some refactoring because of the JNDI aspects you
mentioned. My main aim is to reduce code duplication.
>> - Add optional PA (as in callers can opt to use it if they wish)
>> support to bindThread() and unbindThread()
>
> What do you mean by "PA"?
PrivilegedAction
>> - Switch all the components currently implementing their own
>> bind/unbind methods to use these new context methods
>> - Switch container event type to use an enum
>
> Enums are better than Strings for performance,
> but I am not sure whether those can be extended if needed.
>
> I'd prefer to keep Strings here.
Do we need to extend these? This is all internal to Tomcat. We can add
values to the enum as easily as we can use new String values.
>> - Add bind and unbind events to this new enum
>> - Remove ThreadBindingListener
>>
>> The main benefit of this is that all the actions required on bind and
>> unbind will be in a single place rather than duplicated (sometimes
>> inconsistently) across the code base.
This is really what I am aiming at. There are several places across the
code that do call near enough exactly the code to bind and unbind the
context class loader. It is this duplication I want to remove.
I'll start again and address my primary concern - the duplicate code.
The other aspects I'm happy to think about / discuss further with a view
to possibly coming back to them later.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Rémy Maucherat <re...@apache.org>.
2014/1/22 Konstantin Kolinko <kn...@gmail.com>
> Thus I think that the calls to ContextBindings.bindThread(...) /
> unbindThread(...) as performed by those StandardContext methods are
> needed only during some initial set up. Using them as general methods
> would be a waste. Those methods are overly synchronized and may be a
> nuisance.
>
> It is true these two are only needed during start/stop, and they do more
than what is otherwise needed.
On the overuse of container events, they are called for configuration
changes and (for whatever reason) all session updates. The session updates
indeed mean setting one listener is a rather big impact for meany
applications (if they use lots of session attributes). Ultimately, I added
a dedicated thing because it is a thing EE needs so it got graduated to
something more visible and straightforward to use.
Last [not directly related], looking at the code, I notice that session
expiration already takes care of changing the context CL. I was not aware
of that, and it means I need to drop/rework my SSO patch. Oops. And I'll
also rework the session code since it switches context even if not needed
(in most cases, it is not needed). I'll do that when Mark is done.
Rémy
Re: Context switch integration
Posted by Konstantin Kolinko <kn...@gmail.com>.
2014/1/22 Mark Thomas <ma...@apache.org>:
> On 16/01/2014 15:08, Mark Thomas wrote:
>> On 15/01/2014 15:59, Rémy Maucherat wrote:
>>> 2014/1/15 Mark Thomas <ma...@apache.org>
>>>
>>>>
>>>> If folks integrating with Tomcat need to extend / replace what is
>>>> currently in StandardContext.bindThread() and
>>>> StandardContext.unbindThread() having a listener for this rather than
>>>> having to extend StandardContext makes sense to me.
>>>>
>>>
>>> Ok.
>>>
>>>>
>>>> I'm not sure I follow what you are proposing for PAs.
>>>>
>>>> Nothing, if a utility class was doing the PA itself to make it easier and
>>> do PA + setContextCL + call the listener, there would be more PAs (maybe a
>>> performance impact).
>>
>> Thanks for the clarification. No objections here.
>
> I've been looking at this some more with an eye to reducing code
> duplication. I think the thread binding listener could be merged with
> the ContainerListener. I can't see a good reason to keep them apart at
> this point.
1. If there are many ContainerListeners installed you will be calling
them with an event that they are not interested to handle. It wastes
time.
> With this in mind, the changes I'm planning are:
>
> - Add bindThread() and unbindThread() to the Context interface
Do you mean as a generic replacement to Thread.setContextClassLoader() calls?
I think the current methods of StandardContext are not suitable for
such exposure.
AFAIK, JNDI works in the following way:
1) org.apache.naming.java.javaURLContextFactory class is configured
as the value of System.getProperty(Context.INITIAL_CONTEXT_FACTORY
).
It is normally done by Catalina.initNaming() or by Tomcat.enableNaming()
2) javax.naming.spi.NamingManager.getInitialContext() calls
javaURLContextFactory.getInitialContext()
and it returns an instance of org.apache.naming.SelectorContext
3) SelectorContext uses either current thread or TCCL to find a JNDI
context in a lookup table.
Thus I think that the calls to ContextBindings.bindThread(...) /
unbindThread(...) as performed by those StandardContext methods are
needed only during some initial set up. Using them as general methods
would be a waste. Those methods are overly synchronized and may be a
nuisance.
> - Add optional PA (as in callers can opt to use it if they wish)
> support to bindThread() and unbindThread()
What do you mean by "PA"?
> - Switch all the components currently implementing their own
> bind/unbind methods to use these new context methods
> - Switch container event type to use an enum
Enums are better than Strings for performance,
but I am not sure whether those can be extended if needed.
I'd prefer to keep Strings here.
> - Add bind and unbind events to this new enum
> - Remove ThreadBindingListener
>
> The main benefit of this is that all the actions required on bind and
> unbind will be in a single place rather than duplicated (sometimes
> inconsistently) across the code base.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
On 16/01/2014 15:08, Mark Thomas wrote:
> On 15/01/2014 15:59, Rémy Maucherat wrote:
>> 2014/1/15 Mark Thomas <ma...@apache.org>
>>
>>>
>>> If folks integrating with Tomcat need to extend / replace what is
>>> currently in StandardContext.bindThread() and
>>> StandardContext.unbindThread() having a listener for this rather than
>>> having to extend StandardContext makes sense to me.
>>>
>>
>> Ok.
>>
>>>
>>> I'm not sure I follow what you are proposing for PAs.
>>>
>>> Nothing, if a utility class was doing the PA itself to make it easier and
>> do PA + setContextCL + call the listener, there would be more PAs (maybe a
>> performance impact).
>
> Thanks for the clarification. No objections here.
I've been looking at this some more with an eye to reducing code
duplication. I think the thread binding listener could be merged with
the ContainerListener. I can't see a good reason to keep them apart at
this point. With this in mind, the changes I'm planning are:
- Add bindThread() and unbindThread() to the Context interface
- Add optional PA (as in callers can opt to use it if they wish)
support to bindThread() and unbindThread()
- Switch all the components currently implementing their own
bind/unbind methods to use these new context methods
- Switch container event type to use an enum
- Add bind and unbind events to this new enum
- Remove ThreadBindingListener
The main benefit of this is that all the actions required on bind and
unbind will be in a single place rather than duplicated (sometimes
inconsistently) across the code base.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
On 15/01/2014 15:59, Rémy Maucherat wrote:
> 2014/1/15 Mark Thomas <ma...@apache.org>
>
>>
>> If folks integrating with Tomcat need to extend / replace what is
>> currently in StandardContext.bindThread() and
>> StandardContext.unbindThread() having a listener for this rather than
>> having to extend StandardContext makes sense to me.
>>
>
> Ok.
>
>>
>> I'm not sure I follow what you are proposing for PAs.
>>
>> Nothing, if a utility class was doing the PA itself to make it easier and
> do PA + setContextCL + call the listener, there would be more PAs (maybe a
> performance impact).
Thanks for the clarification. No objections here.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Context switch integration
Posted by Rémy Maucherat <re...@apache.org>.
2014/1/15 Mark Thomas <ma...@apache.org>
>
> If folks integrating with Tomcat need to extend / replace what is
> currently in StandardContext.bindThread() and
> StandardContext.unbindThread() having a listener for this rather than
> having to extend StandardContext makes sense to me.
>
Ok.
>
> I'm not sure I follow what you are proposing for PAs.
>
> Nothing, if a utility class was doing the PA itself to make it easier and
do PA + setContextCL + call the listener, there would be more PAs (maybe a
performance impact).
Rémy
Re: Context switch integration
Posted by Mark Thomas <ma...@apache.org>.
On 14/01/2014 17:03, Rémy Maucherat wrote:
> Hi,
>
> In Tomcat, entering a context is done simply by setting the context
> classloader and sometimes firing some random events (like the ones from the
> ServletRequestListener). This isn't so good for integration and could use
> improvements, in addition to at least one place where this is not done at
> all (very recently a bug reported to me that indicated there was an issue
> with SSO session expiration, and indeed it expires sessions from other
> contexts just like that).
>
> So I would propose harmonizing this (and fix the SSO issue too, obviously),
> with a new dedicated ThreadBindingListener interface (two methods:
> bind/unbind) and a get/set for it also in Context, to be used for
> integration. Although usually generic listeners are used (and there can be
> multiple ones), this has worked well for me so far.
>
> Ideally I would have liked to also take over the privileged actions in a
> utility class, but this would make PA use less optimal (for example in the
> request dispatcher).
>
> Comments ? (or better ideas ?)
If folks integrating with Tomcat need to extend / replace what is
currently in StandardContext.bindThread() and
StandardContext.unbindThread() having a listener for this rather than
having to extend StandardContext makes sense to me.
I'm not sure I follow what you are proposing for PAs.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org