Re: Context switch integration
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
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
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
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
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
2014/1/22 Konstantin Kolinko knst.koli...@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
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
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 signature.asc Description: OpenPGP digital signature
Re: Context switch integration
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
-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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
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
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Context switch integration
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
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
