Available here: https://github.com/infinispan/infinispan/pull/278 The problem is basically that infinispan currently is using TCCL for all class loading and resource loading. This has a lot of problems in modular containers (OSGi, AS7, etc), where you dont have framework implementation classes on the same classloader as user classes (that is how they achieve true isolation) You can read about this in more detail here: http://community.jboss.org/wiki/ModuleCompatibleClassloadingGuide The patch in the pull request is a first step, and should fix many issues, but it does not address all (there is still a lot of TCCL usage spread out among cacheloaders and so on), and ultimately it's just a work around. It should, however, be compatible in any other non-modular environment. Really the ultimate solution is to setup a proper demarcation between what the user is supposed to provide, and what is expected to be bundled with infinispan. Whenever there is something the user can provide a class, then the API should accept a classloader to load that class from.

If it's a class that is just internal wiring of infinispan, then Infinispan's defining classloader should always be used. The one case I can think of in infnispan where TCCL really makes sense is in the case of lazy deserialization from an EE application. However that is only guaranteed to work when you are executing in a context that has that style of contract (like in an EE component). There are many other cases though where someone would expect it to work from a non-EE context (e.g. from a thread pool). What you really want is the caller's classloader, but that is not cheap to get at, so it's something that should be supplied as part of the API interaction (in the case where there is no EE context). Alternatively you can just just require that folks push/pop TCCL, but users often get this wrong. Thanks! -- Jason T. Greene JBoss, a division of Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 4/28/11 4:52 PM, Manik Surtani wrote: > > On 27 Apr 2011, at 12:39, Jason T. Greene wrote: > >> Available here: >> https://github.com/infinispan/infinispan/pull/278 >> >> The problem is basically that infinispan currently is using TCCL for all >> class loading and resource loading. This has a lot of problems in >> modular containers (OSGi, AS7, etc), where you dont have framework >> implementation classes on the same classloader as user classes (that is >> how they achieve true isolation) >> >> You can read about this in more detail here: >> http://community.jboss.org/wiki/ModuleCompatibleClassloadingGuide >> >> The patch in the pull request is a first step, and should fix many >> issues, but it does not address all (there is still a lot of TCCL usage >> spread out among cacheloaders and so on), and ultimately it's just a >> work around. It should, however, be compatible in any other non-modular >> environment. >> >> Really the ultimate solution is to setup a proper demarcation between >> what the user is supposed to provide, and what is expected to be bundled >> with infinispan. Whenever there is something the user can provide a >> class, then the API should accept a classloader to load that class from. > > As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? Not for put, since you have the class, just get, and I was thinking something more like: Foo foo = getUsing(key, Foo.class)

This could verify type compatibility and use the classloader of the type. But that does of course mean more methods on AdvancedCache (not so great). Alternatively you could just have a method on the invocation context. For friendlier interaction and elimination of thread locals, you could introduce a session based API. CacheSession session = CacheSession.from(cache); session.setClassLoader(blah); session.setOtherInterestingOptionsLikeBatchingEtc session.get/put etc -- Jason T. Greene JBoss, a division of Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? > > Not for put, since you have the class, just get, and I was thinking > something more like: > > Foo foo = getUsing(key, Foo.class) This would be a pretty useful addition to the API anyway to avoid user casts.

On 1 May 2011, at 13:38, Pete Muir wrote: >>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >> >> Not for put, since you have the class, just get, and I was thinking >> something more like: >> >> Foo foo = getUsing(key, Foo.class) > > This would be a pretty useful addition to the API anyway to avoid user casts. Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key);

I think Sanne's idea was that you shouldn't have to specify the class or class loader on every get, as it's very likely that all operations on that cache will use the same classloader. Most applications will only use one classloader anyway. I'm not even sure we should allow using more than one classloader, otherwise a get operation might return an object loaded from the wrong classloader. After all, we will only use the provided classloader if we need to get the object from another node or if storeAsBinary is set to true.

Dan On Mon, May 2, 2011 at 11:24 PM, Thomas P. Fuller <thomas.fuller(a)coherentlogic.com&gt; wrote: > How about just using "as" and "using" instead of "asClass" and > "usingClassloader"? > > Consider something like the following which kind of looks like a dsl: > > Foo f = cache.getAdvancedCache().as (Foo.class).using (classLoader).get > (key); > > T > > ________________________________ > From: Sanne Grinovero <sanne.grinovero(a)gmail.com&gt; > To: infinispan -Dev List <infinispan-dev(a)lists.jboss.org&gt; > Sent: Mon, 2 May, 2011 21:08:47 > Subject: Re: [infinispan-dev] [Pull Request] Modular Classloading > Compatibility > > 2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: >> >> On 1 May 2011, at 13:38, Pete Muir wrote: >> >>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>> >>>> Not for put, since you have the class, just get, and I was thinking >>>> something more like: >>>> >>>> Foo foo = getUsing(key, Foo.class) >>> >>> This would be a pretty useful addition to the API anyway to avoid user >>> casts. >> >> Maybe as an "advanced" API, so as not to pollute the basic API? A bit >> like: >> >> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); > > doesn't look much better than a cast, but is more cryptical :) > > getting back to the classloader issue, what about: > > Cache c = cacheManager.getCache( cacheName, classLoader ); > > or > Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); > > BTW if that's an issue on the API, maybe you should propose it to > JSR-107 as well ? > > Sanne > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: > > On 1 May 2011, at 13:38, Pete Muir wrote: > >>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>> >>> Not for put, since you have the class, just get, and I was thinking >>> something more like: >>> >>> Foo foo = getUsing(key, Foo.class) >> >> This would be a pretty useful addition to the API anyway to avoid user casts. > > Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: > > Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); doesn't look much better than a cast, but is more cryptical :) getting back to the classloader issue, what about: Cache c = cacheManager.getCache( cacheName, classLoader ); or Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); BTW if that's an issue on the API, maybe you should propose it to JSR-107 as well ?

2011/5/3 "이희승 (Trustin Lee)" <trustin(a)gmail.com&gt;: > On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >> 2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: >>> >>> On 1 May 2011, at 13:38, Pete Muir wrote: >>> >>>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>>> >>>>> Not for put, since you have the class, just get, and I was thinking >>>>> something more like: >>>>> >>>>> Foo foo = getUsing(key, Foo.class) >>>> >>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>> >>> Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: >>> >>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >> >> doesn't look much better than a cast, but is more cryptical :) >> >> getting back to the classloader issue, what about: >> >> Cache c = cacheManager.getCache( cacheName, classLoader ); >> >> or >> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >> >> BTW if that's an issue on the API, maybe you should propose it to >> JSR-107 as well ? > > We have a configurable Marshaller, right? Then why don't we just use > the class loader that the current Marshaller uses? +1 I like the clean approach, not sure how you configure the "current Marshaller" to use the correct CL ? Likely hard to do via configuration file :)

On 05/03/2011 09:33 PM, Sanne Grinovero wrote: > 2011/5/3 "이희승 (Trustin Lee)" <trustin(a)gmail.com&gt;: >> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>> 2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: >>>> >>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>> >>>>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>>>> >>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>> something more like: >>>>>> >>>>>> Foo foo = getUsing(key, Foo.class) >>>>> >>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>> >>>> Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: >>>> >>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>> >>> doesn't look much better than a cast, but is more cryptical :) >>> >>> getting back to the classloader issue, what about: >>> >>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>> >>> or >>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>> >>> BTW if that's an issue on the API, maybe you should propose it to >>> JSR-107 as well ? >> >> We have a configurable Marshaller, right? Then why don't we just use >> the class loader that the current Marshaller uses? > > +1 > I like the clean approach, not sure how you configure the "current > Marshaller" to use the correct CL ? > Likely hard to do via configuration file :) By default, we could use the class loader that the current Unmarshaller uses, and let user choose a class loader for a certain get() call. So, we need to deal with the two issues here: 1) Make sure that user can configure the default class loader in runtime and calling get() (and consequently related unmarshaller) will use the default class loader: cache.setDefaultClassLoader(UserClass.class.getClassLoader()) 2) Provide an additional API that allows a user to specify a class loader for the current transaction or context: cache.get(K key, Class<V> clazz) // +1 to Pete's suggestion

On 4 May 2011, at 09:55, Dan Berindei wrote: > On Wed, May 4, 2011 at 7:18 AM, "이희승 (Trustin > Lee)&quot;&lt;trustin(a)gmail.com&gt; wrote: >> On 05/03/2011 09:33 PM, Sanne Grinovero wrote: >>> 2011/5/3 "이희승 (Trustin Lee)&quot;&lt;trustin(a)gmail.com&gt;: >>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>> 2011/5/2 Manik Surtani&lt;manik(a)jboss.org&gt;: >>>>>> >>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>> >>>>>>>>> As in, user API? That's a little intrusive... >>>>>>>>> e.g., put(K, V, cl) ? >>>>>>>> >>>>>>>> Not for put, since you have the class, just get, and >>>>>>>> I was thinking something more like: >>>>>>>> >>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>> >>>>>>> This would be a pretty useful addition to the API >>>>>>> anyway to avoid user casts. >>>>>> >>>>>> Maybe as an "advanced" API, so as not to pollute the >>>>>> basic API? A bit like: >>>>>> >>>>>> Foo f = >>>>>> cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>> >>>>> doesn't look much better than a cast, but is more cryptical >>>>> :) >>>>> >>>>> getting back to the classloader issue, what about: >>>>> >>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>> >>>>> or Cache c = cacheManager.getCache( cacheName >>>>> ).usingClassLoader(classLoader ); >>>>> >>>>> BTW if that's an issue on the API, maybe you should propose >>>>> it to JSR-107 as well ? >>>> >>>> We have a configurable Marshaller, right? Then why don't we >>>> just use the class loader that the current Marshaller uses? >>> >>> +1 I like the clean approach, not sure how you configure the >>> "current Marshaller" to use the correct CL ? Likely hard to do >>> via configuration file :) >> >> By default, we could use the class loader that the current >> Unmarshaller uses, and let user choose a class loader for a >> certain get() call. >> >> So, we need to deal with the two issues here: >> >> 1) Make sure that user can configure the default class loader in >> runtime and calling get() (and consequently related unmarshaller) >> will use the default class loader: >> >> cache.setDefaultClassLoader(UserClass.class.getClassLoader()) >> >> 2) Provide an additional API that allows a user to specify a >> class loader for the current transaction or context: >> >> cache.get(K key, Class<V> clazz) // +1 to Pete's suggestion >> > > If V is Set<MyObject> then Set<MyObject>.class.getClassLoader() > will give you the system classloader, which in most cases won't be > able the MyObject class. It may not even be a Set<MyObject> of > course, it could be just Set<?>. > > We could have a "shortcut" so the users can pass in a class instead > of a classloader to avoid writing ".getClassLoader()", but we > shouldn't require any relation between the class passed in and the > class of the return value. There are two forces at play here. Firstly the desire to introduce greater type safety into the Cache API, by returning a type for user, rather than just an object, and secondly the need to specify the CL. We could have an API like: <V, CL> V get(Object key, Class<CL> classLoaderType);

and an overloaded form <V> V get(Object key, ClassLoader cl); This does involve Infinispan having to do a runtime cast to V without the class type passed in as a parameter, but this may be a necessary evil in the interest of improving the user api.

An alternative which is somewhat more verbose, somewhat safer, and somewhat more complex to explain (but ultimately more logical): // Use the classloader of the expectedType, this would be the normal case <V> V get(Object key, Class<V> expectedType); // Specify the classloader to use as a class <V, CL> V get(Object key, Class<V> expectedType, Class<CL>

// Specify the classloader to use as a classloader <V> V get(Object key, Class<V> expectedType, ClassLoader classLoader); We could also include // Use the TCCL V get(Object key);

On 4 May 2011, at 09:55, Dan Berindei wrote: > On Wed, May 4, 2011 at 7:18 AM, "이희승 (Trustin Lee)" <trustin(a)gmail.com&gt; wrote: >> On 05/03/2011 09:33 PM, Sanne Grinovero wrote: >>> 2011/5/3 "이희승 (Trustin Lee)" <trustin(a)gmail.com&gt;: >>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>> 2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: >>>>>> >>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>> >>>>>>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>>>>>> >>>>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>>>> something more like: >>>>>>>> >>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>> >>>>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>>>> >>>>>> Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: >>>>>> >>>>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>> >>>>> doesn't look much better than a cast, but is more cryptical :) >>>>> >>>>> getting back to the classloader issue, what about: >>>>> >>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>> >>>>> or >>>>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>>>> >>>>> BTW if that's an issue on the API, maybe you should propose it to >>>>> JSR-107 as well ? >>>> >>>> We have a configurable Marshaller, right? Then why don't we just use >>>> the class loader that the current Marshaller uses? >>> >>> +1 >>> I like the clean approach, not sure how you configure the "current >>> Marshaller" to use the correct CL ? >>> Likely hard to do via configuration file :) >> >> By default, we could use the class loader that the current Unmarshaller >> uses, and let user choose a class loader for a certain get() call. >> >> So, we need to deal with the two issues here: >> >> 1) Make sure that user can configure the default class loader in runtime >> and calling get() (and consequently related unmarshaller) will use the >> default class loader: >> >> cache.setDefaultClassLoader(UserClass.class.getClassLoader()) >> >> 2) Provide an additional API that allows a user to specify a class >> loader for the current transaction or context: >> >> cache.get(K key, Class<V> clazz) // +1 to Pete's suggestion >> > > If V is Set<MyObject> then Set<MyObject>.class.getClassLoader() will > give you the system classloader, which in most cases won't be able the > MyObject class. It may not even be a Set<MyObject> of course, it could > be just Set<?>. > > We could have a "shortcut" so the users can pass in a class instead of > a classloader to avoid writing ".getClassLoader()", but we shouldn't > require any relation between the class passed in and the class of the > return value. There are two forces at play here. Firstly the desire to introduce greater type safety into the Cache API, by returning a type for user, rather than just an object, and secondly the need to specify the CL. We could have an API like: <V, CL> V get(Object key, Class<CL> classLoaderType); and an overloaded form <V> V get(Object key, ClassLoader cl); This does involve Infinispan having to do a runtime cast to V without the class type passed in as a parameter, but this may be a necessary evil in the interest of improving the user api. An alternative which is somewhat more verbose, somewhat safer, and somewhat more complex to explain (but ultimately more logical): // Use the classloader of the expectedType, this would be the normal case <V> V get(Object key, Class<V> expectedType); // Specify the classloader to use as a class <V, CL> V get(Object key, Class<V> expectedType, Class<CL> classLoader); // Specify the classloader to use as a classloader <V> V get(Object key, Class<V> expectedType, ClassLoader classLoader); We could also include // Use the TCCL V get(Object key); Any thoughts?

If V is Set<MyObject> then Set<MyObject>.class.getClassLoader() will give you the system classloader, which in most cases won't be able the MyObject class. It may not even be a Set<MyObject> of course, it could be just Set<?>. We could have a "shortcut" so the users can pass in a class instead of a classloader to avoid writing ".getClassLoader()", but we shouldn't require any relation between the class passed in and the class of the return value.

On May 3, 2011, at 2:33 PM, Sanne Grinovero wrote: > 2011/5/3 "이희승 (Trustin Lee)" <trustin(a)gmail.com&gt;: >> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>> 2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: >>>> >>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>> >>>>>>> As in, user API?  That's a little intrusive... e.g., put(K, V, cl) ? >>>>>> >>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>> something more like: >>>>>> >>>>>> Foo foo = getUsing(key, Foo.class) >>>>> >>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>> >>>> Maybe as an "advanced" API, so as not to pollute the basic API?  A bit like: >>>> >>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>> >>> doesn't look much better than a cast, but is more cryptical :) >>> >>> getting back to the classloader issue, what about: >>> >>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>> >>> or >>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>> >>> BTW if that's an issue on the API, maybe you should propose it to >>> JSR-107 as well ? >> >> We have a configurable Marshaller, right?  Then why don't we just use >> the class loader that the current Marshaller uses? > > +1 > I like the clean approach, not sure how you configure the "current > Marshaller" to use the correct CL ? > Likely hard to do via configuration file :) Well, the marshaller is a global component and so it's a cache manager level. You can't make any assumptions about it's classloader, particularly when lazy deserialization is configured and you want to make sure that the data of the cache is deserialized with the correct classloader when the user reads the data from the cache. This is gonna become even more important when we for example move to having a single cache for all 2LC entities or all EJB3 SFSBs where we'll definitely need multiple classloaders to access a single cache.

On 4 May 2011, at 05:34, Dan Berindei wrote: > On Wed, May 4, 2011 at 10:14 AM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >> >> On May 3, 2011, at 2:33 PM, Sanne Grinovero wrote: >> >>> 2011/5/3 "이희승 (Trustin Lee)" <trustin(a)gmail.com&gt;: >>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>> 2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: >>>>>> >>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>> >>>>>>>>> As in, user API?  That's a little intrusive... e.g., put(K, V, cl) ? >>>>>>>> >>>>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>>>> something more like: >>>>>>>> >>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>> >>>>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>>>> >>>>>> Maybe as an "advanced" API, so as not to pollute the basic API?  A bit like: >>>>>> >>>>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>> >>>>> doesn't look much better than a cast, but is more cryptical :) >>>>> >>>>> getting back to the classloader issue, what about: >>>>> >>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>> >>>>> or >>>>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>>>> >>>>> BTW if that's an issue on the API, maybe you should propose it to >>>>> JSR-107 as well ? >>>> >>>> We have a configurable Marshaller, right?  Then why don't we just use >>>> the class loader that the current Marshaller uses? >>> >>> +1 >>> I like the clean approach, not sure how you configure the "current >>> Marshaller" to use the correct CL ? >>> Likely hard to do via configuration file :) >> >> Well, the marshaller is a global component and so it's a cache manager level. You can't make any assumptions about it's classloader, particularly when lazy deserialization is configured and you want to make sure that the data of the cache is deserialized with the correct classloader when the user reads the data from the cache. This is gonna become even more important when we for example move to having a single cache for all 2LC entities or all EJB3 SFSBs where we'll definitely need multiple classloaders to access a single cache. >> > > The current unmarshaller uses the TCCL, which is a great idea for > non-modular environments and will still work in AS7 for application > classes (so it's still a good default). It probably won't work if > Hibernate wants to store its own classes in the cache, because > Hibernate's internal classes may not be reachable from the > application's classloader. > > It gets even trickier if Hibernate wants to store a > PrivateHibernateCollection class in the cache containing instances of > application classes inside. Then I don't think there will be any > single classloader that could reach both the Hibernate classes and the > application classes so it can properly unmarshal both. Perhaps that's > just something for the Hibernate folks to worry about? Or maybe we > should allow the users to register more than one classloader with a > cache? You need to use a bridge classloader in this case.

I talked about this with Emmanuel last night, and we were a) concerned about the pollution of the API that this implies b) not sure why we need to do this So I also spoke to Jason to understand his initial motivation, and from this chat I think it's clear that things have gone off course here. Jason raised two issues with modularity/classloading in Infinispan: 1) Using the TCCL as the classloader to load Infinispan classes is a bad idea. Instead we should use org.infinispan.foo.Bar.getClass().getClassLoader(). 2) When we need to load application classes we need a different approach to that used today. Most of the time the TCCL is perfect for this. However *sometimes* Infinispan may be invoked outside of the application, when the TCCL may not be set in AS7. Jason and I discussed three options: a) require (through a platform integration documentation contract) that the TCCL must always be set when Infinispan is invoked. b) Have some sort of InvocationContext which knows what the correct classloader to use is (aka Hibernate/Seam/Weld design where there is a per-application construct based on a ThreadLocal). Given this hasn't been designed into the core, it seems like a large retrofit c) Make users specify the CL (directly or indirectly) via the API (as we discussed). Personally I think that (a) is the best approach right now, and is not that onerous a requirement. We might want to make the TCCL usage pluggable for OSGI envs. Cc'd David to get his feedback.

I talked about this with Emmanuel last night, and we were a) concerned about the pollution of the API that this implies b) not sure why we need to do this So I also spoke to Jason to understand his initial motivation, and from this chat I think it's clear that things have gone off course here. Jason raised two issues with modularity/classloading in Infinispan: 1) Using the TCCL as the classloader to load Infinispan classes is a bad idea. Instead we should use org.infinispan.foo.Bar.getClass().getClassLoader(). 2) When we need to load application classes we need a different approach to that used today. Most of the time the TCCL is perfect for this. However *sometimes* Infinispan may be invoked outside of the application, when the TCCL may not be set in AS7. Jason and I discussed three options: a) require (through a platform integration documentation contract) that the TCCL must always be set when Infinispan is invoked. b) Have some sort of InvocationContext which knows what the correct classloader to use is (aka Hibernate/Seam/Weld design where there is a per-application construct based on a ThreadLocal). Given this hasn't been designed into the core, it seems like a large retrofit c) Make users specify the CL (directly or indirectly) via the API (as we discussed). Personally I think that (a) is the best approach right now, and is not that onerous a requirement. We might want to make the TCCL usage pluggable for OSGI envs. Cc'd David to get his feedback. On 4 May 2011, at 10:46, Dan Berindei wrote: > On Wed, May 4, 2011 at 5:09 PM, Pete Muir <pmuir(a)redhat.com&gt; wrote: >> >> On 4 May 2011, at 05:34, Dan Berindei wrote: >> >>> On Wed, May 4, 2011 at 10:14 AM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >>>> >>>> On May 3, 2011, at 2:33 PM, Sanne Grinovero wrote: >>>> >>>>> 2011/5/3 "이희승 (Trustin Lee)" <trustin(a)gmail.com&gt;: >>>>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>>>> 2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: >>>>>>>> >>>>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>>>> >>>>>>>>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>>>>>>>> >>>>>>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>>>>>> something more like: >>>>>>>>>> >>>>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>>>> >>>>>>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>>>>>> >>>>>>>> Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: >>>>>>>> >>>>>>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>>>> >>>>>>> doesn't look much better than a cast, but is more cryptical :) >>>>>>> >>>>>>> getting back to the classloader issue, what about: >>>>>>> >>>>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>>>> >>>>>>> or >>>>>>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>>>>>> >>>>>>> BTW if that's an issue on the API, maybe you should propose it to >>>>>>> JSR-107 as well ? >>>>>> >>>>>> We have a configurable Marshaller, right? Then why don't we just use >>>>>> the class loader that the current Marshaller uses? >>>>> >>>>> +1 >>>>> I like the clean approach, not sure how you configure the "current >>>>> Marshaller" to use the correct CL ? >>>>> Likely hard to do via configuration file :) >>>> >>>> Well, the marshaller is a global component and so it's a cache manager level. You can't make any assumptions about it's classloader, particularly when lazy deserialization is configured and you want to make sure that the data of the cache is deserialized with the correct classloader when the user reads the data from the cache. This is gonna become even more important when we for example move to having a single cache for all 2LC entities or all EJB3 SFSBs where we'll definitely need multiple classloaders to access a single cache. >>>> >>> >>> The current unmarshaller uses the TCCL, which is a great idea for >>> non-modular environments and will still work in AS7 for application >>> classes (so it's still a good default). It probably won't work if >>> Hibernate wants to store its own classes in the cache, because >>> Hibernate's internal classes may not be reachable from the >>> application's classloader. >>> >>> It gets even trickier if Hibernate wants to store a >>> PrivateHibernateCollection class in the cache containing instances of >>> application classes inside. Then I don't think there will be any >>> single classloader that could reach both the Hibernate classes and the >>> application classes so it can properly unmarshal both. Perhaps that's >>> just something for the Hibernate folks to worry about? Or maybe we >>> should allow the users to register more than one classloader with a >>> cache? >> >> You need to use a bridge classloader in this case. > > You're right of course, Hibernate must have received/guessed the > application's classloader and so it is able to create a bridge > classloader that "includes" both. > > And if the application classes include references to classes from > another module(s), the application has to provide a bridge classloader > to Hibernate anyway. > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Quick question. In case of 2.a (ie setting the TCCL), this is a requirement for frameworks and libraries using Infinispan, right? Not / never for a user application using Infinispan (in this case it seems that the application class loader will do the right thing and is set as the TCCL by the AS environment)? Emmanuel On 5 mai 2011, at 10:55, Pete Muir wrote: > I talked about this with Emmanuel last night, and we were > > a) concerned about the pollution of the API that this implies > b) not sure why we need to do this > > So I also spoke to Jason to understand his initial motivation, and from this chat I think it's clear that things have gone off course here. > > Jason raised two issues with modularity/classloading in Infinispan: > > 1) Using the TCCL as the classloader to load Infinispan classes is a bad idea. Instead we should use org.infinispan.foo.Bar.getClass().getClassLoader(). > > 2) When we need to load application classes we need a different approach to that used today. Most of the time the TCCL is perfect for this. However *sometimes* Infinispan may be invoked outside of the application, when the TCCL may not be set in AS7. Jason and I discussed three options: > > a) require (through a platform integration documentation contract) that the TCCL must always be set when Infinispan is invoked. > b) Have some sort of InvocationContext which knows what the correct classloader to use is (aka Hibernate/Seam/Weld design where there is a per-application construct based on a ThreadLocal). Given this hasn't been designed into the core, it seems like a large retrofit > c) Make users specify the CL (directly or indirectly) via the API (as we discussed). > > Personally I think that (a) is the best approach right now, and is not that onerous a requirement. > > We might want to make the TCCL usage pluggable for OSGI envs. Cc'd David to get his feedback. > > On 4 May 2011, at 10:46, Dan Berindei wrote: > >> On Wed, May 4, 2011 at 5:09 PM, Pete Muir <pmuir(a)redhat.com&gt; wrote: >>> >>> On 4 May 2011, at 05:34, Dan Berindei wrote: >>> >>>> On Wed, May 4, 2011 at 10:14 AM, Galder Zamarreño <galder(a)redhat.com&gt; wrote: >>>>> >>>>> On May 3, 2011, at 2:33 PM, Sanne Grinovero wrote: >>>>> >>>>>> 2011/5/3 "이희승 (Trustin Lee)" <trustin(a)gmail.com&gt;: >>>>>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>>>>> 2011/5/2 Manik Surtani <manik(a)jboss.org&gt;: >>>>>>>>> >>>>>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>>>>> >>>>>>>>>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>>>>>>>>> >>>>>>>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>>>>>>> something more like: >>>>>>>>>>> >>>>>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>>>>> >>>>>>>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>>>>>>> >>>>>>>>> Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: >>>>>>>>> >>>>>>>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>>>>> >>>>>>>> doesn't look much better than a cast, but is more cryptical :) >>>>>>>> >>>>>>>> getting back to the classloader issue, what about: >>>>>>>> >>>>>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>>>>> >>>>>>>> or >>>>>>>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>>>>>>> >>>>>>>> BTW if that's an issue on the API, maybe you should propose it to >>>>>>>> JSR-107 as well ? >>>>>>> >>>>>>> We have a configurable Marshaller, right? Then why don't we just use >>>>>>> the class loader that the current Marshaller uses? >>>>>> >>>>>> +1 >>>>>> I like the clean approach, not sure how you configure the "current >>>>>> Marshaller" to use the correct CL ? >>>>>> Likely hard to do via configuration file :) >>>>> >>>>> Well, the marshaller is a global component and so it's a cache manager level. You can't make any assumptions about it's classloader, particularly when lazy deserialization is configured and you want to make sure that the data of the cache is deserialized with the correct classloader when the user reads the data from the cache. This is gonna become even more important when we for example move to having a single cache for all 2LC entities or all EJB3 SFSBs where we'll definitely need multiple classloaders to access a single cache. >>>>> >>>> >>>> The current unmarshaller uses the TCCL, which is a great idea for >>>> non-modular environments and will still work in AS7 for application >>>> classes (so it's still a good default). It probably won't work if >>>> Hibernate wants to store its own classes in the cache, because >>>> Hibernate's internal classes may not be reachable from the >>>> application's classloader. >>>> >>>> It gets even trickier if Hibernate wants to store a >>>> PrivateHibernateCollection class in the cache containing instances of >>>> application classes inside. Then I don't think there will be any >>>> single classloader that could reach both the Hibernate classes and the >>>> application classes so it can properly unmarshal both. Perhaps that's >>>> just something for the Hibernate folks to worry about? Or maybe we >>>> should allow the users to register more than one classloader with a >>>> cache? >>> >>> You need to use a bridge classloader in this case. >> >> You're right of course, Hibernate must have received/guessed the >> application's classloader and so it is able to create a bridge >> classloader that "includes" both. >> >> And if the application classes include references to classes from >> another module(s), the application has to provide a bridge classloader >> to Hibernate anyway. >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

If you have a Bundle object or BundleContext object you can figure out what classloader is associated with that. Also, if you have a class from a bundle you can find out what it's classloader is (obviously). However, you need to have one of those things to find this out. There is nothing magically available in the current context to tell you what the current Bundle is. It's generally really easy for the caller though to find out what the classloader is, though... If you have bundle X, you'd know a class from that bundle a.b.c.D (any class will do). Then you can simply call D.class.getClassLoader() and you've got the Bundle Classloader. Hope this helps, David On 09/05/2011 16:27, Pete Muir wrote: > The issue that David raises is that in an OSGi env that this wouldn't be done by OSGi so would be up to the user or would require some extra integration library. I'm not even sure if this is possible in OSGi? David, is there anyway for a framework to "find out" the current bundle classloader in OSGi rather than having to be told it (i.e. push rather than pull)? > > The idea is that in AS that by doing (a) it would require the integration to make sure the TCCL is set before Infinispan is called (this is the way many things are integrated into GF for example). > > On 5 May 2011, at 20:05, Emmanuel Bernard wrote: > >> Quick question. >> In case of 2.a (ie setting the TCCL), this is a requirement for frameworks and libraries using Infinispan, right? Not / never for a user application using Infinispan (in this case it seems that the application class loader will do the right thing and is set as the TCCL by the AS environment)? >> >> Emmanuel >> >> On 5 mai 2011, at 10:55, Pete Muir wrote: >> >>> I talked about this with Emmanuel last night, and we were >>> >>> a) concerned about the pollution of the API that this implies >>> b) not sure why we need to do this >>> >>> So I also spoke to Jason to understand his initial motivation, and from this chat I think it's clear that things have gone off course here. >>> >>> Jason raised two issues with modularity/classloading in Infinispan: >>> >>> 1) Using the TCCL as the classloader to load Infinispan classes is a bad idea. Instead we should use org.infinispan.foo.Bar.getClass().getClassLoader(). >>> >>> 2) When we need to load application classes we need a different approach to that used today. Most of the time the TCCL is perfect for this. However *sometimes* Infinispan may be invoked outside of the application, when the TCCL may not be set in AS7. Jason and I discussed three options: >>> >>> a) require (through a platform integration documentation contract) that the TCCL must always be set when Infinispan is invoked. >>> b) Have some sort of InvocationContext which knows what the correct classloader to use is (aka Hibernate/Seam/Weld design where there is a per-application construct based on a ThreadLocal). Given this hasn't been designed into the core, it seems like a large retrofit >>> c) Make users specify the CL (directly or indirectly) via the API (as we discussed). >>> >>> Personally I think that (a) is the best approach right now, and is not that onerous a requirement. >>> >>> We might want to make the TCCL usage pluggable for OSGI envs. Cc'd David to get his feedback. >>> >>> On 4 May 2011, at 10:46, Dan Berindei wrote: >>> >>>> On Wed, May 4, 2011 at 5:09 PM, Pete Muir&lt;pmuir(a)redhat.com&gt; wrote: >>>>> On 4 May 2011, at 05:34, Dan Berindei wrote: >>>>> >>>>>> On Wed, May 4, 2011 at 10:14 AM, Galder Zamarreño&lt;galder(a)redhat.com&gt; wrote: >>>>>>> On May 3, 2011, at 2:33 PM, Sanne Grinovero wrote: >>>>>>> >>>>>>>> 2011/5/3 "이희승 (Trustin Lee)&quot;&lt;trustin(a)gmail.com&gt;: >>>>>>>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>>>>>>> 2011/5/2 Manik Surtani&lt;manik(a)jboss.org&gt;: >>>>>>>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>>>>>>> >>>>>>>>>>>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>>>>>>>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>>>>>>>>> something more like: >>>>>>>>>>>>> >>>>>>>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>>>>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>>>>>>>>> Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: >>>>>>>>>>> >>>>>>>>>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>>>>>>> doesn't look much better than a cast, but is more cryptical :) >>>>>>>>>> >>>>>>>>>> getting back to the classloader issue, what about: >>>>>>>>>> >>>>>>>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>>>>>>> >>>>>>>>>> or >>>>>>>>>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>>>>>>>>> >>>>>>>>>> BTW if that's an issue on the API, maybe you should propose it to >>>>>>>>>> JSR-107 as well ? >>>>>>>>> We have a configurable Marshaller, right? Then why don't we just use >>>>>>>>> the class loader that the current Marshaller uses? >>>>>>>> +1 >>>>>>>> I like the clean approach, not sure how you configure the "current >>>>>>>> Marshaller" to use the correct CL ? >>>>>>>> Likely hard to do via configuration file :) >>>>>>> Well, the marshaller is a global component and so it's a cache manager level. You can't make any assumptions about it's classloader, particularly when lazy deserialization is configured and you want to make sure that the data of the cache is deserialized with the correct classloader when the user reads the data from the cache. This is gonna become even more important when we for example move to having a single cache for all 2LC entities or all EJB3 SFSBs where we'll definitely need multiple classloaders to access a single cache. >>>>>>> >>>>>> The current unmarshaller uses the TCCL, which is a great idea for >>>>>> non-modular environments and will still work in AS7 for application >>>>>> classes (so it's still a good default). It probably won't work if >>>>>> Hibernate wants to store its own classes in the cache, because >>>>>> Hibernate's internal classes may not be reachable from the >>>>>> application's classloader. >>>>>> >>>>>> It gets even trickier if Hibernate wants to store a >>>>>> PrivateHibernateCollection class in the cache containing instances of >>>>>> application classes inside. Then I don't think there will be any >>>>>> single classloader that could reach both the Hibernate classes and the >>>>>> application classes so it can properly unmarshal both. Perhaps that's >>>>>> just something for the Hibernate folks to worry about? Or maybe we >>>>>> should allow the users to register more than one classloader with a >>>>>> cache? >>>>> You need to use a bridge classloader in this case. >>>> You're right of course, Hibernate must have received/guessed the >>>> application's classloader and so it is able to create a bridge >>>> classloader that "includes" both. >>>> >>>> And if the application classes include references to classes from >>>> another module(s), the application has to provide a bridge classloader >>>> to Hibernate anyway. >>>> >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev

Guys, It's funny that we're talking about this but this appears to be precisely the use case that we don't support as per user forum post in: http://community.jboss.org/thread/166080?tstart=0 Pete, do we have a JIRA for this? If not, would you mind creating one?

Cheers, On May 9, 2011, at 5:37 PM, Pete Muir wrote: > Ok, let me stew on that ;-) > > On 9 May 2011, at 16:35, David Bosschaert wrote: > >> If you have a Bundle object or BundleContext object you can figure out what classloader is associated with that. Also, if you have a class from a bundle you can find out what it's classloader is (obviously). >> >> However, you need to have one of those things to find this out. There is nothing magically available in the current context to tell you what the current Bundle is. >> It's generally really easy for the caller though to find out what the classloader is, though... If you have bundle X, you'd know a class from that bundle a.b.c.D (any class will do). Then you can simply call D.class.getClassLoader() and you've got the Bundle Classloader. >> >> Hope this helps, >> >> David >> >> On 09/05/2011 16:27, Pete Muir wrote: >>> The issue that David raises is that in an OSGi env that this wouldn't be done by OSGi so would be up to the user or would require some extra integration library. I'm not even sure if this is possible in OSGi? David, is there anyway for a framework to "find out" the current bundle classloader in OSGi rather than having to be told it (i.e. push rather than pull)? >>> >>> The idea is that in AS that by doing (a) it would require the integration to make sure the TCCL is set before Infinispan is called (this is the way many things are integrated into GF for example). >>> >>> On 5 May 2011, at 20:05, Emmanuel Bernard wrote: >>> >>>> Quick question. >>>> In case of 2.a (ie setting the TCCL), this is a requirement for frameworks and libraries using Infinispan, right? Not / never for a user application using Infinispan (in this case it seems that the application class loader will do the right thing and is set as the TCCL by the AS environment)? >>>> >>>> Emmanuel >>>> >>>> On 5 mai 2011, at 10:55, Pete Muir wrote: >>>> >>>>> I talked about this with Emmanuel last night, and we were >>>>> >>>>> a) concerned about the pollution of the API that this implies >>>>> b) not sure why we need to do this >>>>> >>>>> So I also spoke to Jason to understand his initial motivation, and from this chat I think it's clear that things have gone off course here. >>>>> >>>>> Jason raised two issues with modularity/classloading in Infinispan: >>>>> >>>>> 1) Using the TCCL as the classloader to load Infinispan classes is a bad idea. Instead we should use org.infinispan.foo.Bar.getClass().getClassLoader(). >>>>> >>>>> 2) When we need to load application classes we need a different approach to that used today. Most of the time the TCCL is perfect for this. However *sometimes* Infinispan may be invoked outside of the application, when the TCCL may not be set in AS7. Jason and I discussed three options: >>>>> >>>>> a) require (through a platform integration documentation contract) that the TCCL must always be set when Infinispan is invoked. >>>>> b) Have some sort of InvocationContext which knows what the correct classloader to use is (aka Hibernate/Seam/Weld design where there is a per-application construct based on a ThreadLocal). Given this hasn't been designed into the core, it seems like a large retrofit >>>>> c) Make users specify the CL (directly or indirectly) via the API (as we discussed). >>>>> >>>>> Personally I think that (a) is the best approach right now, and is not that onerous a requirement. >>>>> >>>>> We might want to make the TCCL usage pluggable for OSGI envs. Cc'd David to get his feedback. >>>>> >>>>> On 4 May 2011, at 10:46, Dan Berindei wrote: >>>>> >>>>>> On Wed, May 4, 2011 at 5:09 PM, Pete Muir&lt;pmuir(a)redhat.com&gt; wrote: >>>>>>> On 4 May 2011, at 05:34, Dan Berindei wrote: >>>>>>> >>>>>>>> On Wed, May 4, 2011 at 10:14 AM, Galder Zamarreño&lt;galder(a)redhat.com&gt; wrote: >>>>>>>>> On May 3, 2011, at 2:33 PM, Sanne Grinovero wrote: >>>>>>>>> >>>>>>>>>> 2011/5/3 "이희승 (Trustin Lee)&quot;&lt;trustin(a)gmail.com&gt;: >>>>>>>>>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>>>>>>>>> 2011/5/2 Manik Surtani&lt;manik(a)jboss.org&gt;: >>>>>>>>>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>>>>>>>>>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>>>>>>>>>>> something more like: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>>>>>>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>>>>>>>>>>> Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: >>>>>>>>>>>>> >>>>>>>>>>>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>>>>>>>>> doesn't look much better than a cast, but is more cryptical :) >>>>>>>>>>>> >>>>>>>>>>>> getting back to the classloader issue, what about: >>>>>>>>>>>> >>>>>>>>>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>>>>>>>>> >>>>>>>>>>>> or >>>>>>>>>>>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>>>>>>>>>>> >>>>>>>>>>>> BTW if that's an issue on the API, maybe you should propose it to >>>>>>>>>>>> JSR-107 as well ? >>>>>>>>>>> We have a configurable Marshaller, right? Then why don't we just use >>>>>>>>>>> the class loader that the current Marshaller uses? >>>>>>>>>> +1 >>>>>>>>>> I like the clean approach, not sure how you configure the "current >>>>>>>>>> Marshaller" to use the correct CL ? >>>>>>>>>> Likely hard to do via configuration file :) >>>>>>>>> Well, the marshaller is a global component and so it's a cache manager level. You can't make any assumptions about it's classloader, particularly when lazy deserialization is configured and you want to make sure that the data of the cache is deserialized with the correct classloader when the user reads the data from the cache. This is gonna become even more important when we for example move to having a single cache for all 2LC entities or all EJB3 SFSBs where we'll definitely need multiple classloaders to access a single cache. >>>>>>>>> >>>>>>>> The current unmarshaller uses the TCCL, which is a great idea for >>>>>>>> non-modular environments and will still work in AS7 for application >>>>>>>> classes (so it's still a good default). It probably won't work if >>>>>>>> Hibernate wants to store its own classes in the cache, because >>>>>>>> Hibernate's internal classes may not be reachable from the >>>>>>>> application's classloader. >>>>>>>> >>>>>>>> It gets even trickier if Hibernate wants to store a >>>>>>>> PrivateHibernateCollection class in the cache containing instances of >>>>>>>> application classes inside. Then I don't think there will be any >>>>>>>> single classloader that could reach both the Hibernate classes and the >>>>>>>> application classes so it can properly unmarshal both. Perhaps that's >>>>>>>> just something for the Hibernate folks to worry about? Or maybe we >>>>>>>> should allow the users to register more than one classloader with a >>>>>>>> cache? >>>>>>> You need to use a bridge classloader in this case. >>>>>> You're right of course, Hibernate must have received/guessed the >>>>>> application's classloader and so it is able to create a bridge >>>>>> classloader that "includes" both. >>>>>> >>>>>> And if the application classes include references to classes from >>>>>> another module(s), the application has to provide a bridge classloader >>>>>> to Hibernate anyway. >>>>>> >>>>>> _______________________________________________ >>>>>> infinispan-dev mailing list >>>>>> infinispan-dev(a)lists.jboss.org >>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>> >>>>> _______________________________________________ >>>>> infinispan-dev mailing list >>>>> infinispan-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>> >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Guys, It's funny that we're talking about this but this appears to be precisely the use case that we don't support as per user forum post in: http://community.jboss.org/thread/166080?tstart=0 Pete, do we have a JIRA for this? If not, would you mind creating one? Cheers, On May 9, 2011, at 5:37 PM, Pete Muir wrote: > Ok, let me stew on that ;-) > > On 9 May 2011, at 16:35, David Bosschaert wrote: > >> If you have a Bundle object or BundleContext object you can figure out what classloader is associated with that. Also, if you have a class from a bundle you can find out what it's classloader is (obviously). >> >> However, you need to have one of those things to find this out. There is nothing magically available in the current context to tell you what the current Bundle is. >> It's generally really easy for the caller though to find out what the classloader is, though... If you have bundle X, you'd know a class from that bundle a.b.c.D (any class will do). Then you can simply call D.class.getClassLoader() and you've got the Bundle Classloader. >> >> Hope this helps, >> >> David >> >> On 09/05/2011 16:27, Pete Muir wrote: >>> The issue that David raises is that in an OSGi env that this wouldn't be done by OSGi so would be up to the user or would require some extra integration library. I'm not even sure if this is possible in OSGi? David, is there anyway for a framework to "find out" the current bundle classloader in OSGi rather than having to be told it (i.e. push rather than pull)? >>> >>> The idea is that in AS that by doing (a) it would require the integration to make sure the TCCL is set before Infinispan is called (this is the way many things are integrated into GF for example). >>> >>> On 5 May 2011, at 20:05, Emmanuel Bernard wrote: >>> >>>> Quick question. >>>> In case of 2.a (ie setting the TCCL), this is a requirement for frameworks and libraries using Infinispan, right? Not / never for a user application using Infinispan (in this case it seems that the application class loader will do the right thing and is set as the TCCL by the AS environment)? >>>> >>>> Emmanuel >>>> >>>> On 5 mai 2011, at 10:55, Pete Muir wrote: >>>> >>>>> I talked about this with Emmanuel last night, and we were >>>>> >>>>> a) concerned about the pollution of the API that this implies >>>>> b) not sure why we need to do this >>>>> >>>>> So I also spoke to Jason to understand his initial motivation, and from this chat I think it's clear that things have gone off course here. >>>>> >>>>> Jason raised two issues with modularity/classloading in Infinispan: >>>>> >>>>> 1) Using the TCCL as the classloader to load Infinispan classes is a bad idea. Instead we should use org.infinispan.foo.Bar.getClass().getClassLoader(). >>>>> >>>>> 2) When we need to load application classes we need a different approach to that used today. Most of the time the TCCL is perfect for this. However *sometimes* Infinispan may be invoked outside of the application, when the TCCL may not be set in AS7. Jason and I discussed three options: >>>>> >>>>> a) require (through a platform integration documentation contract) that the TCCL must always be set when Infinispan is invoked. >>>>> b) Have some sort of InvocationContext which knows what the correct classloader to use is (aka Hibernate/Seam/Weld design where there is a per-application construct based on a ThreadLocal). Given this hasn't been designed into the core, it seems like a large retrofit >>>>> c) Make users specify the CL (directly or indirectly) via the API (as we discussed). >>>>> >>>>> Personally I think that (a) is the best approach right now, and is not that onerous a requirement. >>>>> >>>>> We might want to make the TCCL usage pluggable for OSGI envs. Cc'd David to get his feedback. >>>>> >>>>> On 4 May 2011, at 10:46, Dan Berindei wrote: >>>>> >>>>>> On Wed, May 4, 2011 at 5:09 PM, Pete Muir&lt;pmuir(a)redhat.com&gt; wrote: >>>>>>> On 4 May 2011, at 05:34, Dan Berindei wrote: >>>>>>> >>>>>>>> On Wed, May 4, 2011 at 10:14 AM, Galder Zamarreño&lt;galder(a)redhat.com&gt; wrote: >>>>>>>>> On May 3, 2011, at 2:33 PM, Sanne Grinovero wrote: >>>>>>>>> >>>>>>>>>> 2011/5/3 "이희승 (Trustin Lee)&quot;&lt;trustin(a)gmail.com&gt;: >>>>>>>>>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>>>>>>>>> 2011/5/2 Manik Surtani&lt;manik(a)jboss.org&gt;: >>>>>>>>>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>>>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >>>>>>>>>>>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>>>>>>>>>>> something more like: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>>>>>>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>>>>>>>>>>> Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: >>>>>>>>>>>>> >>>>>>>>>>>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>>>>>>>>> doesn't look much better than a cast, but is more cryptical :) >>>>>>>>>>>> >>>>>>>>>>>> getting back to the classloader issue, what about: >>>>>>>>>>>> >>>>>>>>>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>>>>>>>>> >>>>>>>>>>>> or >>>>>>>>>>>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>>>>>>>>>>> >>>>>>>>>>>> BTW if that's an issue on the API, maybe you should propose it to >>>>>>>>>>>> JSR-107 as well ? >>>>>>>>>>> We have a configurable Marshaller, right? Then why don't we just use >>>>>>>>>>> the class loader that the current Marshaller uses? >>>>>>>>>> +1 >>>>>>>>>> I like the clean approach, not sure how you configure the "current >>>>>>>>>> Marshaller" to use the correct CL ? >>>>>>>>>> Likely hard to do via configuration file :) >>>>>>>>> Well, the marshaller is a global component and so it's a cache manager level. You can't make any assumptions about it's classloader, particularly when lazy deserialization is configured and you want to make sure that the data of the cache is deserialized with the correct classloader when the user reads the data from the cache. This is gonna become even more important when we for example move to having a single cache for all 2LC entities or all EJB3 SFSBs where we'll definitely need multiple classloaders to access a single cache. >>>>>>>>> >>>>>>>> The current unmarshaller uses the TCCL, which is a great idea for >>>>>>>> non-modular environments and will still work in AS7 for application >>>>>>>> classes (so it's still a good default). It probably won't work if >>>>>>>> Hibernate wants to store its own classes in the cache, because >>>>>>>> Hibernate's internal classes may not be reachable from the >>>>>>>> application's classloader. >>>>>>>> >>>>>>>> It gets even trickier if Hibernate wants to store a >>>>>>>> PrivateHibernateCollection class in the cache containing instances of >>>>>>>> application classes inside. Then I don't think there will be any >>>>>>>> single classloader that could reach both the Hibernate classes and the >>>>>>>> application classes so it can properly unmarshal both. Perhaps that's >>>>>>>> just something for the Hibernate folks to worry about? Or maybe we >>>>>>>> should allow the users to register more than one classloader with a >>>>>>>> cache? >>>>>>> You need to use a bridge classloader in this case. >>>>>> You're right of course, Hibernate must have received/guessed the >>>>>> application's classloader and so it is able to create a bridge >>>>>> classloader that "includes" both. >>>>>> >>>>>> And if the application classes include references to classes from >>>>>> another module(s), the application has to provide a bridge classloader >>>>>> to Hibernate anyway. >>>>>> >>>>>> _______________________________________________ >>>>>> infinispan-dev mailing list >>>>>> infinispan-dev(a)lists.jboss.org >>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>> _______________________________________________ >>>>> infinispan-dev mailing list >>>>> infinispan-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache

Were we developing for OSGi I would certainly agree with you. However in many environments today we can reasonably expect the TCCL to be set and to be able to load the classes we need. So whilst making it part of the API is the safest option, it's also making complicated an API for the sake of the few at the cost of the many. Further this also seems kinda nasty to me. We know the class (and hence bundle/module) when we put the object into Infinispan, therefore why do we require people to respecify this again? David, can we not actually do something here akin to what we are discussing for Weld? Whereby we can serialize out the bundle id and then find the correct CL based on that when we deserialize.

On 9 May 2011, at 20:57, David Bosschaert wrote: > Yeah, the bottom line is that the value of the TCCL is not defined by > the OSGi standards. You can define it yourself by setting it before you > call the relevant code but as said before this is ugly and it's actually > very error-prone because there's nothing in the API to remind you to do > it, so you might forget the odd time (it's effectively a hidden argument > to the API). > To make things even worse, there is a chance that the TCCL is not unset > after previous use which means that its value might happen to be correct > in some cases and incorrect in others - which is what the issue in the > post seems to be hitting. > > I think making this part of the API explicit is the safest option. Yes, > it requires people to think about classloading who don't like to think > about classloading, but generally passing in getClass().getClassLoader() > is all that's needed to get the right one... > > Just my 2c, > > David > > On 09/05/2011 18:01, Galder Zamarreño wrote: >> Guys, >> >> It's funny that we're talking about this but this appears to be precisely the use case that we don't support as per user forum post in: http://community.jboss.org/thread/166080?tstart=0 >> >> Pete, do we have a JIRA for this? If not, would you mind creating one? >> >> Cheers, >> >> On May 9, 2011, at 5:37 PM, Pete Muir wrote: >> >>> Ok, let me stew on that ;-) >>> >>> On 9 May 2011, at 16:35, David Bosschaert wrote: >>> >>>> If you have a Bundle object or BundleContext object you can figure out what classloader is associated with that. Also, if you have a class from a bundle you can find out what it's classloader is (obviously). >>>> >>>> However, you need to have one of those things to find this out. There is nothing magically available in the current context to tell you what the current Bundle is. >>>> It's generally really easy for the caller though to find out what the classloader is, though... If you have bundle X, you'd know a class from that bundle a.b.c.D (any class will do). Then you can simply call D.class.getClassLoader() and you've got the Bundle Classloader. >>>> >>>> Hope this helps, >>>> >>>> David >>>> >>>> On 09/05/2011 16:27, Pete Muir wrote: >>>>> The issue that David raises is that in an OSGi env that this wouldn't be done by OSGi so would be up to the user or would require some extra integration library. I'm not even sure if this is possible in OSGi? David, is there anyway for a framework to "find out" the current bundle classloader in OSGi rather than having to be told it (i.e. push rather than pull)? >>>>> >>>>> The idea is that in AS that by doing (a) it would require the integration to make sure the TCCL is set before Infinispan is called (this is the way many things are integrated into GF for example). >>>>> >>>>> On 5 May 2011, at 20:05, Emmanuel Bernard wrote: >>>>> >>>>>> Quick question. >>>>>> In case of 2.a (ie setting the TCCL), this is a requirement for frameworks and libraries using Infinispan, right? Not / never for a user application using Infinispan (in this case it seems that the application class loader will do the right thing and is set as the TCCL by the AS environment)? >>>>>> >>>>>> Emmanuel >>>>>> >>>>>> On 5 mai 2011, at 10:55, Pete Muir wrote: >>>>>> >>>>>>> I talked about this with Emmanuel last night, and we were >>>>>>> >>>>>>> a) concerned about the pollution of the API that this implies >>>>>>> b) not sure why we need to do this >>>>>>> >>>>>>> So I also spoke to Jason to understand his initial motivation, and from this chat I think it's clear that things have gone off course here. >>>>>>> >>>>>>> Jason raised two issues with modularity/classloading in Infinispan: >>>>>>> >>>>>>> 1) Using the TCCL as the classloader to load Infinispan classes is a bad idea. Instead we should use org.infinispan.foo.Bar.getClass().getClassLoader(). >>>>>>> >>>>>>> 2) When we need to load application classes we need a different approach to that used today. Most of the time the TCCL is perfect for this. However *sometimes* Infinispan may be invoked outside of the application, when the TCCL may not be set in AS7. Jason and I discussed three options: >>>>>>> >>>>>>> a) require (through a platform integration documentation contract) that the TCCL must always be set when Infinispan is invoked. >>>>>>> b) Have some sort of InvocationContext which knows what the correct classloader to use is (aka Hibernate/Seam/Weld design where there is a per-application construct based on a ThreadLocal). Given this hasn't been designed into the core, it seems like a large retrofit >>>>>>> c) Make users specify the CL (directly or indirectly) via the API (as we discussed). >>>>>>> >>>>>>> Personally I think that (a) is the best approach right now, and is not that onerous a requirement. >>>>>>> >>>>>>> We might want to make the TCCL usage pluggable for OSGI envs. Cc'd David to get his feedback. >>>>>>> >>>>>>> On 4 May 2011, at 10:46, Dan Berindei wrote: >>>>>>> >>>>>>>> On Wed, May 4, 2011 at 5:09 PM, Pete Muir&lt;pmuir(a)redhat.com&gt;   wrote: >>>>>>>>> On 4 May 2011, at 05:34, Dan Berindei wrote: >>>>>>>>> >>>>>>>>>> On Wed, May 4, 2011 at 10:14 AM, Galder Zamarreño&lt;galder(a)redhat.com&gt;   wrote: >>>>>>>>>>> On May 3, 2011, at 2:33 PM, Sanne Grinovero wrote: >>>>>>>>>>> >>>>>>>>>>>> 2011/5/3 "이희승 (Trustin Lee)&quot;&lt;trustin(a)gmail.com&gt;: >>>>>>>>>>>>> On 05/03/2011 05:08 AM, Sanne Grinovero wrote: >>>>>>>>>>>>>> 2011/5/2 Manik Surtani&lt;manik(a)jboss.org&gt;: >>>>>>>>>>>>>>> On 1 May 2011, at 13:38, Pete Muir wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> As in, user API?  That's a little intrusive... e.g., put(K, V, cl) ? >>>>>>>>>>>>>>>>> Not for put, since you have the class, just get, and I was thinking >>>>>>>>>>>>>>>>> something more like: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Foo foo = getUsing(key, Foo.class) >>>>>>>>>>>>>>>> This would be a pretty useful addition to the API anyway to avoid user casts. >>>>>>>>>>>>>>> Maybe as an "advanced" API, so as not to pollute the basic API?  A bit like: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key); >>>>>>>>>>>>>> doesn't look much better than a cast, but is more cryptical :) >>>>>>>>>>>>>> >>>>>>>>>>>>>> getting back to the classloader issue, what about: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Cache c = cacheManager.getCache( cacheName, classLoader ); >>>>>>>>>>>>>> >>>>>>>>>>>>>> or >>>>>>>>>>>>>> Cache c = cacheManager.getCache( cacheName ).usingClassLoader(classLoader ); >>>>>>>>>>>>>> >>>>>>>>>>>>>> BTW if that's an issue on the API, maybe you should propose it to >>>>>>>>>>>>>> JSR-107 as well ? >>>>>>>>>>>>> We have a configurable Marshaller, right?  Then why don't we just use >>>>>>>>>>>>> the class loader that the current Marshaller uses? >>>>>>>>>>>> +1 >>>>>>>>>>>> I like the clean approach, not sure how you configure the "current >>>>>>>>>>>> Marshaller" to use the correct CL ? >>>>>>>>>>>> Likely hard to do via configuration file :) >>>>>>>>>>> Well, the marshaller is a global component and so it's a cache manager level. You can't make any assumptions about it's classloader, particularly when lazy deserialization is configured and you want to make sure that the data of the cache is deserialized with the correct classloader when the user reads the data from the cache. This is gonna become even more important when we for example move to having a single cache for all 2LC entities or all EJB3 SFSBs where we'll definitely need multiple classloaders to access a single cache. >>>>>>>>>>> >>>>>>>>>> The current unmarshaller uses the TCCL, which is a great idea for >>>>>>>>>> non-modular environments and will still work in AS7 for application >>>>>>>>>> classes (so it's still a good default). It probably won't work if >>>>>>>>>> Hibernate wants to store its own classes in the cache, because >>>>>>>>>> Hibernate's internal classes may not be reachable from the >>>>>>>>>> application's classloader. >>>>>>>>>> >>>>>>>>>> It gets even trickier if Hibernate wants to store a >>>>>>>>>> PrivateHibernateCollection class in the cache containing instances of >>>>>>>>>> application classes inside. Then I don't think there will be any >>>>>>>>>> single classloader that could reach both the Hibernate classes and the >>>>>>>>>> application classes so it can properly unmarshal both. Perhaps that's >>>>>>>>>> just something for the Hibernate folks to worry about? Or maybe we >>>>>>>>>> should allow the users to register more than one classloader with a >>>>>>>>>> cache? >>>>>>>>> You need to use a bridge classloader in this case. >>>>>>>> You're right of course, Hibernate must have received/guessed the >>>>>>>> application's classloader and so it is able to create a bridge >>>>>>>> classloader that "includes" both. >>>>>>>> >>>>>>>> And if the application classes include references to classes from >>>>>>>> another module(s), the application has to provide a bridge classloader >>>>>>>> to Hibernate anyway. >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> infinispan-dev mailing list >>>>>>>> infinispan-dev(a)lists.jboss.org >>>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>>>> _______________________________________________ >>>>>>> infinispan-dev mailing list >>>>>>> infinispan-dev(a)lists.jboss.org >>>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>>> _______________________________________________ >>>>>> infinispan-dev mailing list >>>>>> infinispan-dev(a)lists.jboss.org >>>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>>>> _______________________________________________ >>>>> infinispan-dev mailing list >>>>> infinispan-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> -- >> Galder Zamarreño >> Sr. Software Engineer >> Infinispan, JBoss Cache >> > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 11/05/2011 17:54, Dan Berindei wrote: > On Wed, May 11, 2011 at 7:08 PM, Pete Muir&lt;pmuir(a)redhat.com&gt;  wrote: >> Were we developing for OSGi I would certainly agree with you. However in many environments today we can reasonably expect the TCCL to be set and to be able to load the classes we need. So whilst making it part of the API is the safest option, it's also making complicated an API for the sake of the few at the cost of the many. Further this also seems kinda nasty to me. We know the class (and hence bundle/module) when we put the object into Infinispan, therefore why do we require people to respecify this again? >> >> David, can we not actually do something here akin to what we are discussing for Weld? Whereby we can serialize out the bundle id and then find the correct CL based on that when we deserialize. > What if the object is a java.util.ArrayList? Each element in the list > could belong to a different bundle, so you'd have to write a bundle id > for every element in the list. Yes, if you know the Bundle-SymbolicName and Version (or the Bundle ID) you can find its classloader. On the other question, if you're passing in a class object then you can obtain its classloader and hence the bundle where it came from. But, and I think this is what Dan allused to above, is it always true that the class your passing in comes from the bundle that you need to have or could it also come from one of its parent class loaders?

On 12 mai 2011, at 11:18, Dan Berindei wrote: > That seems like a lot of overhead to me, and forcing the user to > provide the classloader doesn't seem that bad in comparison. Perhaps > we should use something other than a thread-local for this though, so > that users can do a onto the result of a > cacheManager.getCache("A").usingClassLoader(A.class) and never have to > provide the classloader again. Note that I don't think it works in the case you mentioned earlier ie a ArrayList with classes from many different bundles.

On 5/12/11 4:18 AM, Dan Berindei wrote: > > On Wed, May 11, 2011 at 11:18 PM, David Bosschaert&lt;david(a)redhat.com&gt; >  wrote: >> >> On 11/05/2011 17:54, Dan Berindei wrote: >>> >>> On Wed, May 11, 2011 at 7:08 PM, Pete Muir&lt;pmuir(a)redhat.com&gt;    wrote: >>>> >>>> Were we developing for OSGi I would certainly agree with you. However >>>> in many environments today we can reasonably expect the TCCL to be set and >>>> to be able to load the classes we need. So whilst making it part of the API >>>> is the safest option, it's also making complicated an API for the sake of >>>> the few at the cost of the many. Further this also seems kinda nasty to me. >>>> We know the class (and hence bundle/module) when we put the object into >>>> Infinispan, therefore why do we require people to respecify this again? >>>> >>>> David, can we not actually do something here akin to what we are >>>> discussing for Weld? Whereby we can serialize out the bundle id and then >>>> find the correct CL based on that when we deserialize. >>> >>> What if the object is a java.util.ArrayList? Each element in the list >>> could belong to a different bundle, so you'd have to write a bundle id >>> for every element in the list. >> >> Yes, if you know the Bundle-SymbolicName and Version (or the Bundle ID) >> you can find its classloader. >> >> On the other question, if you're passing in a class object then you can >> obtain its classloader and hence the bundle where it came from. But, and >> I think this is what Dan allused to above, is it always true that the >> class your passing in comes from the bundle that you need to have or >> could it also come from one of its parent class loaders? >> > > Exactly David, sorry if my message was a little cryptic. I think in > order to handle every case properly you would have to go through the > entire object graph being stored in the cache in order to find all the > classloaders/bundle ids that you will need on get(). This approach just doesn't scale, and it would not work in all environments (there is no gaurantee you can get the original classloader if the environment doesn't allow you to look them up by some kind of unique id). > That seems like a lot of overhead to me, and forcing the user to > provide the classloader doesn't seem that bad in comparison. Perhaps > we should use something other than a thread-local for this though Yes thread locals are brittle, and tend to leak if they aren't managed correctly. Using a context specific instance is a much better approach. > so > that users can do a onto the result of a > cacheManager.getCache("A").usingClassLoader(A.class) and never have to > provide the classloader again. > That's similar to what I was proposing with the CacheSession notion. The basic notion is that you move thread/context specific state to a separate object that the caller uses (optionaly), and it mirrors the same Cache API.

> In fact I think this is a good idea for the invocation flags we > already have, too. It would involve creating lots of overloads in > CacheDelegate with a PreInvocationContext parameter and a new > CacheDelegateWithContext class to invoke those methods, but the public > API would remain the same. You dont need to reuse the same impl. Just create a new delegate which passes an extra invocation state parameter with ever call. The overhead of that is tiny.

On May 12, 2011, at 11:18 AM, Dan Berindei wrote: > On Wed, May 11, 2011 at 11:18 PM, David Bosschaert <david(a)redhat.com&gt; wrote: >> On 11/05/2011 17:54, Dan Berindei wrote: >>> On Wed, May 11, 2011 at 7:08 PM, Pete Muir&lt;pmuir(a)redhat.com&gt; wrote: >>>> Were we developing for OSGi I would certainly agree with you. However in many environments today we can reasonably expect the TCCL to be set and to be able to load the classes we need. So whilst making it part of the API is the safest option, it's also making complicated an API for the sake of the few at the cost of the many. Further this also seems kinda nasty to me. We know the class (and hence bundle/module) when we put the object into Infinispan, therefore why do we require people to respecify this again? >>>> >>>> David, can we not actually do something here akin to what we are discussing for Weld? Whereby we can serialize out the bundle id and then find the correct CL based on that when we deserialize. >>> What if the object is a java.util.ArrayList? Each element in the list >>> could belong to a different bundle, so you'd have to write a bundle id >>> for every element in the list. >> Yes, if you know the Bundle-SymbolicName and Version (or the Bundle ID) >> you can find its classloader. >> >> On the other question, if you're passing in a class object then you can >> obtain its classloader and hence the bundle where it came from. But, and >> I think this is what Dan allused to above, is it always true that the >> class your passing in comes from the bundle that you need to have or >> could it also come from one of its parent class loaders? >> > > Exactly David, sorry if my message was a little cryptic. I think in > order to handle every case properly you would have to go through the > entire object graph being stored in the cache in order to find all the > classloaders/bundle ids that you will need on get(). > > That seems like a lot of overhead to me, and forcing the user to > provide the classloader doesn't seem that bad in comparison. Perhaps > we should use something other than a thread-local for this though, so > that users can do a onto the result of a > cacheManager.getCache("A").usingClassLoader(A.class) and never have to > provide the classloader again. > > In fact I think this is a good idea for the invocation flags we > already have, too. It would involve creating lots of overloads in > CacheDelegate with a PreInvocationContext parameter and a new > CacheDelegateWithContext class to invoke those methods, but the public > API would remain the same. No matter how I look at it, putting a classloader in a thread local makes me shiver.

Just imagine the mayhem you can cause if you "forget" to clear the thread local. I've done enough of Apache Commons Logging support to understand that you should limit the references to classloaders to the minimum, particularly in system classes/infrastructure. If we need to end up forcing users to register classloaders in these scenarions, we need to do it in such way that either: - we can detect these leaks (it might be a bit primitive now but old JBoss JCA code had an interesting way of discovering unclosed open connections) - if we give you on trying to detect them, the impact of a leak is reduced as much as possible. Cheers, -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

I don't like the TCCL either, so I'll repeat my suggestion from two weeks ago to just have: Cache c = cacheManager.getCache( cacheName, classLoader ); sounds reasonable to me to have the application declare it's intentions once ? BTW I don't like "cache.get(K key, Class<V> clazz)" as we're not speaking only about the get(K) method, but about many methods and this will explode the number of method of Cache; on the other hand I think it;s acceptable to have a single Cache instance used by a single application/classloader. You can still have multiple applications share the same underlying cache and use different classloaders: getCache( cacheName, classLoader ) would return a delegate to the original cache, having a specific marshaller in the invocation context as Trustin was suggesting. Cheers, Sanne 2011/5/16 Pete Muir&lt;pmuir(a)redhat.com&gt;: > > On 16 May 2011, at 18:20, Galder Zamarreño wrote: > >> >> On May 12, 2011, at 11:18 AM, Dan Berindei wrote: >> >>> On Wed, May 11, 2011 at 11:18 PM, David Bosschaert&lt;david(a)redhat.com&gt; wrote: >>>> On 11/05/2011 17:54, Dan Berindei wrote: >>>>> On Wed, May 11, 2011 at 7:08 PM, Pete Muir&lt;pmuir(a)redhat.com&gt; wrote: >>>>>> Were we developing for OSGi I would certainly agree with you. However in many environments today we can reasonably expect the TCCL to be set and to be able to load the classes we need. So whilst making it part of the API is the safest option, it's also making complicated an API for the sake of the few at the cost of the many. Further this also seems kinda nasty to me. We know the class (and hence bundle/module) when we put the object into Infinispan, therefore why do we require people to respecify this again? >>>>>> >>>>>> David, can we not actually do something here akin to what we are discussing for Weld? Whereby we can serialize out the bundle id and then find the correct CL based on that when we deserialize. >>>>> What if the object is a java.util.ArrayList? Each element in the list >>>>> could belong to a different bundle, so you'd have to write a bundle id >>>>> for every element in the list. >>>> Yes, if you know the Bundle-SymbolicName and Version (or the Bundle ID) >>>> you can find its classloader. >>>> >>>> On the other question, if you're passing in a class object then you can >>>> obtain its classloader and hence the bundle where it came from. But, and >>>> I think this is what Dan allused to above, is it always true that the >>>> class your passing in comes from the bundle that you need to have or >>>> could it also come from one of its parent class loaders? >>>> >>> >>> Exactly David, sorry if my message was a little cryptic. I think in >>> order to handle every case properly you would have to go through the >>> entire object graph being stored in the cache in order to find all the >>> classloaders/bundle ids that you will need on get(). >>> >>> That seems like a lot of overhead to me, and forcing the user to >>> provide the classloader doesn't seem that bad in comparison. Perhaps >>> we should use something other than a thread-local for this though, so >>> that users can do a onto the result of a >>> cacheManager.getCache("A").usingClassLoader(A.class) and never have to >>> provide the classloader again. >>> >>> In fact I think this is a good idea for the invocation flags we >>> already have, too. It would involve creating lots of overloads in >>> CacheDelegate with a PreInvocationContext parameter and a new >>> CacheDelegateWithContext class to invoke those methods, but the public >>> API would remain the same. >> >> No matter how I look at it, putting a classloader in a thread local makes me shiver. > > I also wonder why we want do this, given we already have a construct called the Thread Local Context Classloader ;-) > > Either we use that, or use some other mechanism. > >> Just imagine the mayhem you can cause if you "forget" to clear the thread local. >> >> I've done enough of Apache Commons Logging support to understand that you should limit the references to classloaders to the minimum, particularly in system classes/infrastructure. >> >> If we need to end up forcing users to register classloaders in these scenarions, we need to do it in such way that either: >> >> - we can detect these leaks (it might be a bit primitive now but old JBoss JCA code had an interesting way of discovering unclosed open connections) >> >> - if we give you on trying to detect them, the impact of a leak is reduced as much as possible. >> >> Cheers, >> -- >> Galder Zamarreño >> Sr. Software Engineer >> Infinispan, JBoss Cache >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

I don't like the TCCL either, so I'll repeat my suggestion from two weeks ago to just have: Cache c = cacheManager.getCache( cacheName, classLoader ); sounds reasonable to me to have the application declare it's intentions once ? BTW I don't like "cache.get(K key, Class<V> clazz)" as we're not speaking only about the get(K) method, but about many methods and this will explode the number of method of Cache; on the other hand I think it;s acceptable to have a single Cache instance used by a single application/classloader. You can still have multiple applications share the same underlying cache and use different classloaders: getCache( cacheName, classLoader ) would return a delegate to the original cache, having a specific marshaller in the invocation context as Trustin was suggesting. Cheers, Sanne 2011/5/16 Pete Muir <pmuir(a)redhat.com&gt;: > > On 16 May 2011, at 18:20, Galder Zamarreño wrote: > >> >> On May 12, 2011, at 11:18 AM, Dan Berindei wrote: >> >>> On Wed, May 11, 2011 at 11:18 PM, David Bosschaert <david(a)redhat.com&gt; wrote: >>>> On 11/05/2011 17:54, Dan Berindei wrote: >>>>> On Wed, May 11, 2011 at 7:08 PM, Pete Muir&lt;pmuir(a)redhat.com&gt; wrote: >>>>>> Were we developing for OSGi I would certainly agree with you. However in many environments today we can reasonably expect the TCCL to be set and to be able to load the classes we need. So whilst making it part of the API is the safest option, it's also making complicated an API for the sake of the few at the cost of the many. Further this also seems kinda nasty to me. We know the class (and hence bundle/module) when we put the object into Infinispan, therefore why do we require people to respecify this again? >>>>>> >>>>>> David, can we not actually do something here akin to what we are discussing for Weld? Whereby we can serialize out the bundle id and then find the correct CL based on that when we deserialize. >>>>> What if the object is a java.util.ArrayList? Each element in the list >>>>> could belong to a different bundle, so you'd have to write a bundle id >>>>> for every element in the list. >>>> Yes, if you know the Bundle-SymbolicName and Version (or the Bundle ID) >>>> you can find its classloader. >>>> >>>> On the other question, if you're passing in a class object then you can >>>> obtain its classloader and hence the bundle where it came from. But, and >>>> I think this is what Dan allused to above, is it always true that the >>>> class your passing in comes from the bundle that you need to have or >>>> could it also come from one of its parent class loaders? >>>> >>> >>> Exactly David, sorry if my message was a little cryptic. I think in >>> order to handle every case properly you would have to go through the >>> entire object graph being stored in the cache in order to find all the >>> classloaders/bundle ids that you will need on get(). >>> >>> That seems like a lot of overhead to me, and forcing the user to >>> provide the classloader doesn't seem that bad in comparison. Perhaps >>> we should use something other than a thread-local for this though, so >>> that users can do a onto the result of a >>> cacheManager.getCache("A").usingClassLoader(A.class) and never have to >>> provide the classloader again. >>> >>> In fact I think this is a good idea for the invocation flags we >>> already have, too. It would involve creating lots of overloads in >>> CacheDelegate with a PreInvocationContext parameter and a new >>> CacheDelegateWithContext class to invoke those methods, but the public >>> API would remain the same. >> >> No matter how I look at it, putting a classloader in a thread local makes me shiver. > > I also wonder why we want do this, given we already have a construct called the Thread Local Context Classloader ;-) > > Either we use that, or use some other mechanism. > >> Just imagine the mayhem you can cause if you "forget" to clear the thread local. >> >> I've done enough of Apache Commons Logging support to understand that you should limit the references to classloaders to the minimum, particularly in system classes/infrastructure. >> >> If we need to end up forcing users to register classloaders in these scenarions, we need to do it in such way that either: >> >> - we can detect these leaks (it might be a bit primitive now but old JBoss JCA code had an interesting way of discovering unclosed open connections) >> >> - if we give you on trying to detect them, the impact of a leak is reduced as much as possible. >> >> Cheers, >> -- >> Galder Zamarreño >> Sr. Software Engineer >> Infinispan, JBoss Cache >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

2011/5/16 Pete Muir <pmuir(a)redhat.com&gt;: > This is the "hibernate session style contract" that Jason is talking about. As the CM can be shared (e.g. in JNDI), then the same Cache object can be returned in multiple applications in the app server, meaning you can't simply associate the CL with a Cache object when it's created. A CM could be JNDI registered, but a Cache is not registered. assuming same cacheManager, two different applications could do: cacheA = cm.getCache( "hibernateCache", appAClassLoader ); cacheB = cm.getCache( "hibernateCache", appBClassLoader ); cacheA != cacheB still they will delegate toe the same "hibernateCache", but being different only in which ClassLoader they set in their invocation context, which will be read by the Unmarshaller.

I don't like the TCCL either, so I'll repeat my suggestion from two weeks ago to just have: Cache c = cacheManager.getCache( cacheName, classLoader ); sounds reasonable to me to have the application declare it's intentions once ? BTW I don't like "cache.get(K key, Class<V> clazz)" as we're not speaking only about the get(K) method, but about many methods and this will explode the number of method of Cache; on the other hand I think it;s acceptable to have a single Cache instance used by a single application/classloader. You can still have multiple applications share the same underlying cache and use different classloaders:

getCache( cacheName, classLoader ) would return a delegate to the original cache, having a specific marshaller in the invocation context as Trustin was suggesting. Cheers, Sanne 2011/5/16 Pete Muir <pmuir(a)redhat.com&gt;: > > On 16 May 2011, at 18:20, Galder Zamarreño wrote: > >> >> On May 12, 2011, at 11:18 AM, Dan Berindei wrote: >> >>> On Wed, May 11, 2011 at 11:18 PM, David Bosschaert <david(a)redhat.com&gt; wrote: >>>> On 11/05/2011 17:54, Dan Berindei wrote: >>>>> On Wed, May 11, 2011 at 7:08 PM, Pete Muir&lt;pmuir(a)redhat.com&gt; wrote: >>>>>> Were we developing for OSGi I would certainly agree with you. However in many environments today we can reasonably expect the TCCL to be set and to be able to load the classes we need. So whilst making it part of the API is the safest option, it's also making complicated an API for the sake of the few at the cost of the many. Further this also seems kinda nasty to me. We know the class (and hence bundle/module) when we put the object into Infinispan, therefore why do we require people to respecify this again? >>>>>> >>>>>> David, can we not actually do something here akin to what we are discussing for Weld? Whereby we can serialize out the bundle id and then find the correct CL based on that when we deserialize. >>>>> What if the object is a java.util.ArrayList? Each element in the list >>>>> could belong to a different bundle, so you'd have to write a bundle id >>>>> for every element in the list. >>>> Yes, if you know the Bundle-SymbolicName and Version (or the Bundle ID) >>>> you can find its classloader. >>>> >>>> On the other question, if you're passing in a class object then you can >>>> obtain its classloader and hence the bundle where it came from. But, and >>>> I think this is what Dan allused to above, is it always true that the >>>> class your passing in comes from the bundle that you need to have or >>>> could it also come from one of its parent class loaders? >>>> >>> >>> Exactly David, sorry if my message was a little cryptic. I think in >>> order to handle every case properly you would have to go through the >>> entire object graph being stored in the cache in order to find all the >>> classloaders/bundle ids that you will need on get(). >>> >>> That seems like a lot of overhead to me, and forcing the user to >>> provide the classloader doesn't seem that bad in comparison. Perhaps >>> we should use something other than a thread-local for this though, so >>> that users can do a onto the result of a >>> cacheManager.getCache("A").usingClassLoader(A.class) and never have to >>> provide the classloader again. >>> >>> In fact I think this is a good idea for the invocation flags we >>> already have, too. It would involve creating lots of overloads in >>> CacheDelegate with a PreInvocationContext parameter and a new >>> CacheDelegateWithContext class to invoke those methods, but the public >>> API would remain the same. >> >> No matter how I look at it, putting a classloader in a thread local makes me shiver. > > I also wonder why we want do this, given we already have a construct called the Thread Local Context Classloader ;-) > > Either we use that, or use some other mechanism. > >> Just imagine the mayhem you can cause if you "forget" to clear the thread local. >> >> I've done enough of Apache Commons Logging support to understand that you should limit the references to classloaders to the minimum, particularly in system classes/infrastructure. >> >> If we need to end up forcing users to register classloaders in these scenarions, we need to do it in such way that either: >> >> - we can detect these leaks (it might be a bit primitive now but old JBoss JCA code had an interesting way of discovering unclosed open connections) >> >> - if we give you on trying to detect them, the impact of a leak is reduced as much as possible. >> >> Cheers, >> -- >> Galder Zamarreño >> Sr. Software Engineer >> Infinispan, JBoss Cache >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

What about a helper that just returns a cache with a specific classloader from a cache? cache.withLoader(cl).get(K key) -a On Mon, May 16, 2011 at 11:14 AM, Galder Zamarreño <galder(a)redhat.com <mailto:galder@redhat.com>> wrote: On May 16, 2011, at 7:57 PM, Sanne Grinovero wrote: > I don't like the TCCL either, so I'll repeat my suggestion from two > weeks ago to just have: > > Cache c = cacheManager.getCache( cacheName, classLoader ); > > sounds reasonable to me to have the application declare it's intentions once ? > > BTW I don't like > > "cache.get(K key, Class<V> clazz)" > > as we're not speaking only about the get(K) method, but about many > methods and this will explode the number of method of Cache; on the > other hand I think it;s acceptable to have a single Cache instance > used by a single application/classloader. You can still have multiple > applications share the same underlying cache and use different > classloaders: Guys, we're going around in circles. As I said the other week, you can't assume 1 cache = 1 classloader cos for example in the Hibernate 2LC all entities will be stored in a single cache as opposed to today where we have a cache per entity. And if all entities are stored in the same cache, we potentially have a cache that contains data belonging to multiple cache loaders. And the reason for all this is cos we don't support asymmetric clusters. Could someone start a design wiki to grab all the requirements? > > getCache( cacheName, classLoader ) would return a delegate to the > original cache, having a specific marshaller in the invocation context > as Trustin was suggesting. > > Cheers, > Sanne > > > 2011/5/16 Pete Muir <pmuir(a)redhat.com <mailto:pmuir@redhat.com>>: >> >> On 16 May 2011, at 18:20, Galder Zamarreño wrote: >> >>> >>> On May 12, 2011, at 11:18 AM, Dan Berindei wrote: >>> >>>> On Wed, May 11, 2011 at 11:18 PM, David Bosschaert <david(a)redhat.com <mailto:david@redhat.com>> wrote: >>>>> On 11/05/2011 17:54, Dan Berindei wrote: >>>>>> On Wed, May 11, 2011 at 7:08 PM, Pete Muir&lt;pmuir(a)redhat.com <mailto:pmuir@redhat.com>> wrote: >>>>>>> Were we developing for OSGi I would certainly agree with you. However in many environments today we can reasonably expect the TCCL to be set and to be able to load the classes we need. So whilst making it part of the API is the safest option, it's also making complicated an API for the sake of the few at the cost of the many. Further this also seems kinda nasty to me. We know the class (and hence bundle/module) when we put the object into Infinispan, therefore why do we require people to respecify this again? >>>>>>> >>>>>>> David, can we not actually do something here akin to what we are discussing for Weld? Whereby we can serialize out the bundle id and then find the correct CL based on that when we deserialize. >>>>>> What if the object is a java.util.ArrayList? Each element in the list >>>>>> could belong to a different bundle, so you'd have to write a bundle id >>>>>> for every element in the list. >>>>> Yes, if you know the Bundle-SymbolicName and Version (or the Bundle ID) >>>>> you can find its classloader. >>>>> >>>>> On the other question, if you're passing in a class object then you can >>>>> obtain its classloader and hence the bundle where it came from. But, and >>>>> I think this is what Dan allused to above, is it always true that the >>>>> class your passing in comes from the bundle that you need to have or >>>>> could it also come from one of its parent class loaders? >>>>> >>>> >>>> Exactly David, sorry if my message was a little cryptic. I think in >>>> order to handle every case properly you would have to go through the >>>> entire object graph being stored in the cache in order to find all the >>>> classloaders/bundle ids that you will need on get(). >>>> >>>> That seems like a lot of overhead to me, and forcing the user to >>>> provide the classloader doesn't seem that bad in comparison. Perhaps >>>> we should use something other than a thread-local for this though, so >>>> that users can do a onto the result of a >>>> cacheManager.getCache("A").usingClassLoader(A.class) and never have to >>>> provide the classloader again. >>>> >>>> In fact I think this is a good idea for the invocation flags we >>>> already have, too. It would involve creating lots of overloads in >>>> CacheDelegate with a PreInvocationContext parameter and a new >>>> CacheDelegateWithContext class to invoke those methods, but the public >>>> API would remain the same. >>> >>> No matter how I look at it, putting a classloader in a thread local makes me shiver. >> >> I also wonder why we want do this, given we already have a construct called the Thread Local Context Classloader ;-) >> >> Either we use that, or use some other mechanism. >> >>> Just imagine the mayhem you can cause if you "forget" to clear the thread local. >>> >>> I've done enough of Apache Commons Logging support to understand that you should limit the references to classloaders to the minimum, particularly in system classes/infrastructure. >>> >>> If we need to end up forcing users to register classloaders in these scenarions, we need to do it in such way that either: >>> >>> - we can detect these leaks (it might be a bit primitive now but old JBoss JCA code had an interesting way of discovering unclosed open connections) >>> >>> - if we give you on trying to detect them, the impact of a leak is reduced as much as possible. >>> >>> Cheers, >>> -- >>> Galder Zamarreño >>> Sr. Software Engineer >>> Infinispan, JBoss Cache >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Not to be the guy that breaks the circle, how about the threads that handle incoming RPCs from other nodes? With storeAsBinary="false" those threads have to unmarshal the objects in order to store them on the local node, so we'd need a way to register a classloader with the cache, not just with the cache session. If we use a single cache for all Hibernate entities, regardless of application, this means the cache could receive application objects from the other nodes without the application even being deployed. Does this mean we'll require storeAsBinary="true" in all Hibernate 2LC configurations?

Dan On Mon, May 16, 2011 at 9:54 PM, David M. Lloyd <david.lloyd(a)redhat.com&gt; wrote: > I know, we can just attach the class loader to the cache! > > Okay, just kidding, but Galder is right, this conversation is going in > circles. We already discussed that in this thread and a number of > points were raised for and against. > > On 05/16/2011 01:20 PM, Adrian Cole wrote: >> What about a helper that just returns a cache with a specific >> classloader from a cache? >> >> cache.withLoader(cl).get(K key) >> >> -a >> >> >> On Mon, May 16, 2011 at 11:14 AM, Galder Zamarreño <galder(a)redhat.com >> <mailto:galder@redhat.com>> wrote: >> >> >> On May 16, 2011, at 7:57 PM, Sanne Grinovero wrote: >> >> > I don't like the TCCL either, so I'll repeat my suggestion from two >> > weeks ago to just have: >> > >> > Cache c = cacheManager.getCache( cacheName, classLoader ); >> > >> > sounds reasonable to me to have the application declare it's >> intentions once ? >> > >> > BTW I don't like >> > >> > "cache.get(K key, Class<V> clazz)" >> > >> > as we're not speaking only about the get(K) method, but about many >> > methods and this will explode the number of method of Cache; on the >> > other hand I think it;s acceptable to have a single Cache instance >> > used by a single application/classloader. You can still have multiple >> > applications share the same underlying cache and use different >> > classloaders: >> >> Guys, we're going around in circles. As I said the other week, you >> can't assume 1 cache = 1 classloader cos for example in the >> Hibernate 2LC all entities will be stored in a single cache as >> opposed to today where we have a cache per entity. And if all >> entities are stored in the same cache, we potentially have a cache >> that contains data belonging to multiple cache loaders. And the >> reason for all this is cos we don't support asymmetric clusters. >> >> Could someone start a design wiki to grab all the requirements? >> >> > >> > getCache( cacheName, classLoader ) would return a delegate to the >> > original cache, having a specific marshaller in the invocation >> context >> > as Trustin was suggesting. >> > >> > Cheers, >> > Sanne >> > >> > >> > 2011/5/16 Pete Muir <pmuir(a)redhat.com <mailto:pmuir@redhat.com>>: >> >> >> >> On 16 May 2011, at 18:20, Galder Zamarreño wrote: >> >> >> >>> >> >>> On May 12, 2011, at 11:18 AM, Dan Berindei wrote: >> >>> >> >>>> On Wed, May 11, 2011 at 11:18 PM, David Bosschaert >> <david(a)redhat.com <mailto:david@redhat.com>> wrote: >> >>>>> On 11/05/2011 17:54, Dan Berindei wrote: >> >>>>>> On Wed, May 11, 2011 at 7:08 PM, Pete Muir&lt;pmuir(a)redhat.com >> <mailto:pmuir@redhat.com>> wrote: >> >>>>>>> Were we developing for OSGi I would certainly agree with >> you. However in many environments today we can reasonably expect the >> TCCL to be set and to be able to load the classes we need. So whilst >> making it part of the API is the safest option, it's also making >> complicated an API for the sake of the few at the cost of the many. >> Further this also seems kinda nasty to me. We know the class (and >> hence bundle/module) when we put the object into Infinispan, >> therefore why do we require people to respecify this again? >> >>>>>>> >> >>>>>>> David, can we not actually do something here akin to what >> we are discussing for Weld? Whereby we can serialize out the bundle >> id and then find the correct CL based on that when we deserialize. >> >>>>>> What if the object is a java.util.ArrayList? Each element in >> the list >> >>>>>> could belong to a different bundle, so you'd have to write a >> bundle id >> >>>>>> for every element in the list. >> >>>>> Yes, if you know the Bundle-SymbolicName and Version (or the >> Bundle ID) >> >>>>> you can find its classloader. >> >>>>> >> >>>>> On the other question, if you're passing in a class object >> then you can >> >>>>> obtain its classloader and hence the bundle where it came >> from. But, and >> >>>>> I think this is what Dan allused to above, is it always true >> that the >> >>>>> class your passing in comes from the bundle that you need to >> have or >> >>>>> could it also come from one of its parent class loaders? >> >>>>> >> >>>> >> >>>> Exactly David, sorry if my message was a little cryptic. I >> think in >> >>>> order to handle every case properly you would have to go >> through the >> >>>> entire object graph being stored in the cache in order to find >> all the >> >>>> classloaders/bundle ids that you will need on get(). >> >>>> >> >>>> That seems like a lot of overhead to me, and forcing the user to >> >>>> provide the classloader doesn't seem that bad in comparison. >> Perhaps >> >>>> we should use something other than a thread-local for this >> though, so >> >>>> that users can do a onto the result of a >> >>>> cacheManager.getCache("A").usingClassLoader(A.class) and never >> have to >> >>>> provide the classloader again. >> >>>> >> >>>> In fact I think this is a good idea for the invocation flags we >> >>>> already have, too. It would involve creating lots of overloads in >> >>>> CacheDelegate with a PreInvocationContext parameter and a new >> >>>> CacheDelegateWithContext class to invoke those methods, but >> the public >> >>>> API would remain the same. >> >>> >> >>> No matter how I look at it, putting a classloader in a thread >> local makes me shiver. >> >> >> >> I also wonder why we want do this, given we already have a >> construct called the Thread Local Context Classloader ;-) >> >> >> >> Either we use that, or use some other mechanism. >> >> >> >>> Just imagine the mayhem you can cause if you "forget" to clear >> the thread local. >> >>> >> >>> I've done enough of Apache Commons Logging support to >> understand that you should limit the references to classloaders to >> the minimum, particularly in system classes/infrastructure. >> >>> >> >>> If we need to end up forcing users to register classloaders in >> these scenarions, we need to do it in such way that either: >> >>> >> >>> - we can detect these leaks (it might be a bit primitive now >> but old JBoss JCA code had an interesting way of discovering >> unclosed open connections) >> >>> >> >>> - if we give you on trying to detect them, the impact of a leak >> is reduced as much as possible. >> >>> >> >>> Cheers, >> >>> -- >> >>> Galder Zamarreño >> >>> Sr. Software Engineer >> >>> Infinispan, JBoss Cache >> >>> >> >>> >> >>> _______________________________________________ >> >>> infinispan-dev mailing list >> >>> infinispan-dev(a)lists.jboss.org >> <mailto:infinispan-dev@lists.jboss.org> >> >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> >> >> >> >> _______________________________________________ >> >> infinispan-dev mailing list >> >> infinispan-dev(a)lists.jboss.org >> <mailto:infinispan-dev@lists.jboss.org> >> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> >> > >> > _______________________________________________ >> > infinispan-dev mailing list >> > infinispan-dev(a)lists.jboss.org >> <mailto:infinispan-dev@lists.jboss.org> >> > https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> -- >> Galder Zamarreño >> Sr. Software Engineer >> Infinispan, JBoss Cache >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> >> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > -- > - DML > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 1 May 2011, at 13:38, Pete Muir wrote: >>> As in, user API? That's a little intrusive... e.g., put(K, V, cl) ? >> >> Not for put, since you have the class, just get, and I was thinking >> something more like: >> >> Foo foo = getUsing(key, Foo.class) > > This would be a pretty useful addition to the API anyway to avoid user casts. Maybe as an "advanced" API, so as not to pollute the basic API? A bit like: Foo f = cache.getAdvancedCache().asClass(Foo.class).get(key);

Available here: https://github.com/infinispan/infinispan/pull/278 The problem is basically that infinispan currently is using TCCL for all class loading and resource loading. This has a lot of problems in modular containers (OSGi, AS7, etc), where you dont have framework implementation classes on the same classloader as user classes (that is how they achieve true isolation) You can read about this in more detail here: http://community.jboss.org/wiki/ModuleCompatibleClassloadingGuide The patch in the pull request is a first step, and should fix many issues, but it does not address all (there is still a lot of TCCL usage spread out among cacheloaders and so on), and ultimately it's just a work around. It should, however, be compatible in any other non-modular environment. Really the ultimate solution is to setup a proper demarcation between what the user is supposed to provide, and what is expected to be bundled with infinispan. Whenever there is something the user can provide a class, then the API should accept a classloader to load that class from. If it's a class that is just internal wiring of infinispan, then Infinispan's defining classloader should always be used. The one case I can think of in infnispan where TCCL really makes sense is in the case of lazy deserialization from an EE application. However that is only guaranteed to work when you are executing in a context that has that style of contract (like in an EE component). There are many other cases though where someone would expect it to work from a non-EE context (e.g. from a thread pool). What you really want is the caller's classloader, but that is not cheap to get at, so it's something that should be supplied as part of the API interaction (in the case where there is no EE context). Alternatively you can just just require that folks push/pop TCCL, but users often get this wrong. Thanks! -- Jason T. Greene JBoss, a division of Red Hat _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On Wed, May 18, 2011 at 7:06 PM, Manik Surtani <manik(a)jboss.org&gt; wrote: > Hi guys > > Sorry I've been absent from this thread for a while now (it's been growing faster than I've been able to deal with email backlog!) > > Anyway, this is a very interesting discussion.  To summarise - as Pete did at some point - there are 2 goals here: > > 1.  Safe and intuitive use of an appropriate classloader > 2.  Safe type system for return values. > > I think the far more pressing concern is (1) so I'd like to focus on that.  If we think (2) is pressing enough a concern, we should spawn a separate thread and discuss there. > > So, onto the issue of safe classloading. > > 1) Class loader per session/cache. > > I like Jason/Sanne/Trustin's suggestions of a session-like contract, and specifically I think this is best achieved as a delegate to a cache, again as suggested elsewhere by Pete, etc.  E.g., > >        Cache<?, ?> myCache = cacheManager.getCache("myCache", myClassLoader); > > and what is returned is something that delegates to the actual cache, making sure the TCCL is set and re-set appropriately.  The handle to the cache is effectively your "session" and each webapp, etc in an EE environment will have its own handle.  I propose using the TCCL as an internal implementation detail within this delegate, helps with making sure it is carefully managed and cleaned up while not re-engineering loads of internals. > I like the API but I would not recommend using the TCCL for this. I was able to get a nice perf jump in the HotRod client by skipping 2 Thread.setContextClassLoader() calls on each cache operation (1 to set the TCCL we wanted and 1 to restore the original TCCL). Setting the TCCL is a privileged operation, so it has to go through a SecurityManager and that is very slow. > I think EmbeddedCacheManager.getCache(String name, ClassLoader cl) is enough ... it is clear enough, and I don't see the need for overloaded getCache(name, classOfWhichClassLoaderIWishToUse). > I agree, a Cache.usingClassloader(classOfWhichClassLoaderIWishToUse) overload would have made sense because the method name already communicates the intention, but a getCache(name, clazz) overload is too obscure. > 2) Class loader per invocation. > > I've been less than happy with this, not just because it pollutes the API but that it adds a layer of confusion.  If all use cases discussed can be solved with (1) above, then I'd prefer to just do that. > > The way I see it, most user apps directly using Infinispan would only be exposed to a single class loader per cache reference (even if multiple references talk to the same cache). > > Frameworks, OTOH, are a bit tougher, Hibernate being a good example on this thread.  So this is a question for Galder - is it feasible to maintain several references to a cache, one for each app/persistence unit? > > 3) Can all OSGi requirements be handled by (1)? I would guess so, from what I have read here, since the class loader is explicitly passed in when getting a handle on a cache. > Yes, the only difference is that OSGi goesn't make any guarantees about the TCCL, so passing the classloader explicitly will work in all environments. However, 4) What about storeAsBinary="false"? Threads processing requests from other nodes are not associated with any CacheManager.getCache(name, classloader) call, and they also have to unmarshall values with this setting. Since Hibernate already mandates storeAsBinary="true" for its 2LC, we can probably get away with only supporting one classloader per cache in the storeAsBinary="false" case. Still, we can't rely on the TCCL of the background threads because those threads are shared between all the caches in a CacheManager. In fact we should probably set the TCCL to null for all background threads created by Infinispan, or we risk keeping the classes of the first application/bundle that called us alive as long as those threads are still running. Dan

> Cheers > Manik > > > On 27 Apr 2011, at 17:39, Jason T. Greene wrote: > >> Available here: >> https://github.com/infinispan/infinispan/pull/278 >> >> The problem is basically that infinispan currently is using TCCL for all >> class loading and resource loading. This has a lot of problems in >> modular containers (OSGi, AS7, etc), where you dont have framework >> implementation classes on the same classloader as user classes (that is >> how they achieve true isolation) >> >> You can read about this in more detail here: >> http://community.jboss.org/wiki/ModuleCompatibleClassloadingGuide >> >> The patch in the pull request is a first step, and should fix many >> issues, but it does not address all (there is still a lot of TCCL usage >> spread out among cacheloaders and so on), and ultimately it's just a >> work around. It should, however, be compatible in any other non-modular >> environment. >> >> Really the ultimate solution is to setup a proper demarcation between >> what the user is supposed to provide, and what is expected to be bundled >> with infinispan. Whenever there is something the user can provide a >> class, then the API should accept a classloader to load that class from. >> If it's a class that is just internal wiring of infinispan, then >> Infinispan's defining classloader should always be used. >> >> The one case I can think of in infnispan where TCCL really makes sense >> is in the case of lazy deserialization from an EE application. However >> that is only guaranteed to work when you are executing in a context that >> has that style of contract (like in an EE component). There are many >> other cases though where someone would expect it to work from a non-EE >> context (e.g. from a thread pool). What you really want is the caller's >> classloader, but that is not cheap to get at, so it's something that >> should be supplied as part of the API interaction (in the case where >> there is no EE context). Alternatively you can just just require that >> folks push/pop TCCL, but users often get this wrong. >> >> Thanks! >> >> -- >> Jason T. Greene >> JBoss, a division of Red Hat >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > -- > Manik Surtani > manik(a)jboss.org > twitter.com/maniksurtani > > Lead, Infinispan > http://www.infinispan.org > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev > _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On May 19, 2011, at 10:04 AM, Sanne Grinovero wrote: > 2011/5/19 Dan Berindei <dan.berindei(a)gmail.com&gt;: >> On Wed, May 18, 2011 at 7:06 PM, Manik Surtani <manik(a)jboss.org&gt; wrote: >>> Hi guys >>> >>> Sorry I've been absent from this thread for a while now (it's been growing faster than I've been able to deal with email backlog!) >>> >>> Anyway, this is a very interesting discussion.  To summarise - as Pete did at some point - there are 2 goals here: >>> >>> 1.  Safe and intuitive use of an appropriate classloader >>> 2.  Safe type system for return values. >>> >>> I think the far more pressing concern is (1) so I'd like to focus on that.  If we think (2) is pressing enough a concern, we should spawn a separate thread and discuss there. >>> >>> So, onto the issue of safe classloading. >>> >>> 1) Class loader per session/cache. >>> >>> I like Jason/Sanne/Trustin's suggestions of a session-like contract, and specifically I think this is best achieved as a delegate to a cache, again as suggested elsewhere by Pete, etc.  E.g., >>> >>>        Cache<?, ?> myCache = cacheManager.getCache("myCache", myClassLoader); >>> >>> and what is returned is something that delegates to the actual cache, making sure the TCCL is set and re-set appropriately.  The handle to the cache is effectively your "session" and each webapp, etc in an EE environment will have its own handle.  I propose using the TCCL as an internal implementation detail within this delegate, helps with making sure it is carefully managed and cleaned up while not re-engineering loads of internals. >>> >> >> I like the API but I would not recommend using the TCCL for this. I >> was able to get a nice perf jump in the HotRod client by skipping 2 >> Thread.setContextClassLoader() calls on each cache operation (1 to set >> the TCCL we wanted and 1 to restore the original TCCL). Setting the >> TCCL is a privileged operation, so it has to go through a >> SecurityManager and that is very slow. >> >> >>> I think EmbeddedCacheManager.getCache(String name, ClassLoader cl) is enough ... it is clear enough, and I don't see the need for overloaded getCache(name, classOfWhichClassLoaderIWishToUse). >>> >> >> I agree, a Cache.usingClassloader(classOfWhichClassLoaderIWishToUse) >> overload would have made sense because the method name already >> communicates the intention, but a getCache(name, clazz) overload is >> too obscure. >> >> >>> 2) Class loader per invocation. >>> >>> I've been less than happy with this, not just because it pollutes the API but that it adds a layer of confusion.  If all use cases discussed can be solved with (1) above, then I'd prefer to just do that. >>> >>> The way I see it, most user apps directly using Infinispan would only be exposed to a single class loader per cache reference (even if multiple references talk to the same cache). >>> >>> Frameworks, OTOH, are a bit tougher, Hibernate being a good example on this thread.  So this is a question for Galder - is it feasible to maintain several references to a cache, one for each app/persistence unit? >>> >>> 3) Can all OSGi requirements be handled by (1)? I would guess so, from what I have read here, since the class loader is explicitly passed in when getting a handle on a cache. >>> >> >> Yes, the only difference is that OSGi goesn't make any guarantees >> about the TCCL, so passing the classloader explicitly will work in all >> environments. However, >> >> 4) What about storeAsBinary="false"? Threads processing requests from >> other nodes are not associated with any CacheManager.getCache(name, >> classloader) call, and they also have to unmarshall values with this >> setting. >> >> Since Hibernate already mandates storeAsBinary="true" for its 2LC, we >> can probably get away with only supporting one classloader per cache >> in the storeAsBinary="false" case. >> >> Still, we can't rely on the TCCL of the background threads because >> those threads are shared between all the caches in a CacheManager. In >> fact we should probably set the TCCL to null for all background >> threads created by Infinispan, or we risk keeping the classes of the >> first application/bundle that called us alive as long as those threads >> are still running. >> >> Dan > > Totally agree with all you said, great analysis! > > So it looks like that all Caches being shared across different > classloaders (deployed applications) > should be used only with storeAsBinary="false", still I'm having some > doubts about how even that is safe. > How can we prevent two applications both using hibernate from mixing > the cache keys and values? How do you do that? Apps don't have any direct access to the Hibernate Cache. > I assume the keys being used in the 2LC use entity names or class > names combined with the primary keys; > it's totally possible to have multiple applications using the same > entity names, which is quite common in my experience. That's what region name comes in. Multiple apps shouldn't have the same region name. That's what discriminates entities in different apps.

> > The configurations defined in the app server could be used as > templates to start/stop specific caches for each deployment, > generating a unique name depending on the ear/war/.. in such a way to > be consistent with names on other nodes. > Was this all designed as a workaround for the fact that we're still > having trouble in forming a cluster of cachemanagers having different > caches? > > Sanne > >> >> >>> Cheers >>> Manik >>> >>> >>> On 27 Apr 2011, at 17:39, Jason T. Greene wrote: >>> >>>> Available here: >>>> https://github.com/infinispan/infinispan/pull/278 >>>> >>>> The problem is basically that infinispan currently is using TCCL for all >>>> class loading and resource loading. This has a lot of problems in >>>> modular containers (OSGi, AS7, etc), where you dont have framework >>>> implementation classes on the same classloader as user classes (that is >>>> how they achieve true isolation) >>>> >>>> You can read about this in more detail here: >>>> http://community.jboss.org/wiki/ModuleCompatibleClassloadingGuide >>>> >>>> The patch in the pull request is a first step, and should fix many >>>> issues, but it does not address all (there is still a lot of TCCL usage >>>> spread out among cacheloaders and so on), and ultimately it's just a >>>> work around. It should, however, be compatible in any other non-modular >>>> environment. >>>> >>>> Really the ultimate solution is to setup a proper demarcation between >>>> what the user is supposed to provide, and what is expected to be bundled >>>> with infinispan. Whenever there is something the user can provide a >>>> class, then the API should accept a classloader to load that class from. >>>> If it's a class that is just internal wiring of infinispan, then >>>> Infinispan's defining classloader should always be used. >>>> >>>> The one case I can think of in infnispan where TCCL really makes sense >>>> is in the case of lazy deserialization from an EE application. However >>>> that is only guaranteed to work when you are executing in a context that >>>> has that style of contract (like in an EE component). There are many >>>> other cases though where someone would expect it to work from a non-EE >>>> context (e.g. from a thread pool). What you really want is the caller's >>>> classloader, but that is not cheap to get at, so it's something that >>>> should be supplied as part of the API interaction (in the case where >>>> there is no EE context). Alternatively you can just just require that >>>> folks push/pop TCCL, but users often get this wrong. >>>> >>>> Thanks! >>>> >>>> -- >>>> Jason T. Greene >>>> JBoss, a division of Red Hat >>>> _______________________________________________ >>>> infinispan-dev mailing list >>>> infinispan-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >>> -- >>> Manik Surtani >>> manik(a)jboss.org >>> twitter.com/maniksurtani >>> >>> Lead, Infinispan >>> http://www.infinispan.org >>> >>> >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >>> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Hi guys Sorry I've been absent from this thread for a while now (it's been growing faster than I've been able to deal with email backlog!) Anyway, this is a very interesting discussion. To summarise - as Pete did at some point - there are 2 goals here: 1. Safe and intuitive use of an appropriate classloader 2. Safe type system for return values. I think the far more pressing concern is (1) so I'd like to focus on that. If we think (2) is pressing enough a concern, we should spawn a separate thread and discuss there. So, onto the issue of safe classloading. 1) Class loader per session/cache. I like Jason/Sanne/Trustin's suggestions of a session-like contract, and specifically I think this is best achieved as a delegate to a cache, again as suggested elsewhere by Pete, etc. E.g., Cache<?, ?> myCache = cacheManager.getCache("myCache", myClassLoader); and what is returned is something that delegates to the actual cache, making sure the TCCL is set and re-set appropriately. The handle to the cache is effectively your "session" and each webapp, etc in an EE environment will have its own handle. I propose using the TCCL as an internal implementation detail within this delegate, helps with making sure it is carefully managed and cleaned up while not re-engineering loads of internals. I think EmbeddedCacheManager.getCache(String name, ClassLoader cl) is enough ... it is clear enough, and I don't see the need for overloaded getCache(name, classOfWhichClassLoaderIWishToUse).

On May 18, 2011, at 6:06 PM, Manik Surtani wrote: > 2) Class loader per invocation. > > I've been less than happy with this, not just because it pollutes the API but that it adds a layer of confusion. If all use cases discussed can be solved with (1) above, then I'd prefer to just do that. > > The way I see it, most user apps directly using Infinispan would only be exposed to a single class loader per cache reference (even if multiple references talk to the same cache). > > Frameworks, OTOH, are a bit tougher, Hibernate being a good example on this thread. So this is a question for Galder - is it feasible to maintain several references to a cache, one for each app/persistence unit? Sorry, missed the second part of the email, forget it my questions are irrelevant after this. I think this might be doable. Each entity/collection region has a cache associated which is built of the region factory. As long as when the region's classloader can be identified at that point and passed to the cache manager on cache retrieval, we should be Ok.

Otherwise, storeAsBinary would be the alternative for these cases. > > 3) Can all OSGi requirements be handled by (1)? I would guess so, from what I have read here, since the class loader is explicitly passed in when getting a handle on a cache. > > Cheers > Manik > > > On 27 Apr 2011, at 17:39, Jason T. Greene wrote: > >> Available here: >> https://github.com/infinispan/infinispan/pull/278 >> >> The problem is basically that infinispan currently is using TCCL for all >> class loading and resource loading. This has a lot of problems in >> modular containers (OSGi, AS7, etc), where you dont have framework >> implementation classes on the same classloader as user classes (that is >> how they achieve true isolation) >> >> You can read about this in more detail here: >> http://community.jboss.org/wiki/ModuleCompatibleClassloadingGuide >> >> The patch in the pull request is a first step, and should fix many >> issues, but it does not address all (there is still a lot of TCCL usage >> spread out among cacheloaders and so on), and ultimately it's just a >> work around. It should, however, be compatible in any other non-modular >> environment. >> >> Really the ultimate solution is to setup a proper demarcation between >> what the user is supposed to provide, and what is expected to be bundled >> with infinispan. Whenever there is something the user can provide a >> class, then the API should accept a classloader to load that class from. >> If it's a class that is just internal wiring of infinispan, then >> Infinispan's defining classloader should always be used. >> >> The one case I can think of in infnispan where TCCL really makes sense >> is in the case of lazy deserialization from an EE application. However >> that is only guaranteed to work when you are executing in a context that >> has that style of contract (like in an EE component). There are many >> other cases though where someone would expect it to work from a non-EE >> context (e.g. from a thread pool). What you really want is the caller's >> classloader, but that is not cheap to get at, so it's something that >> should be supplied as part of the API interaction (in the case where >> there is no EE context). Alternatively you can just just require that >> folks push/pop TCCL, but users often get this wrong. >> >> Thanks! >> >> -- >> Jason T. Greene >> JBoss, a division of Red Hat >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > -- > Manik Surtani > manik(a)jboss.org > twitter.com/maniksurtani > > Lead, Infinispan > http://www.infinispan.org > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

would be different cache instances. The problem then is that if an RPC comes for "entities" cache and entity P1, which of the "entities" caches do I go for? You'd need to know which classloader P1 is living in the remote node and you'd have to now that at the Infinispan level to be able to store it in a non-binary format.

On May 19, 2011, at 12:11 PM, Manik Surtani wrote: > Guys - what are we talking about?  Specifying ClassLoaders is only meaningful if storeAsBinary is set to true. Ok, that was not clear to me throughout the discussion.

> In general, any situation where you have code booted off different ClassLoaders running in the same JVM and sharing the same cache (or cache manager), you would *need* to set storeAsBinary to true to get around deserialization issues on remote nodes. > > StoreAsBinary = false only really works for trivial cases where caches/cache managers run in environments where only one cache loader is in effect.  I.e., *not* JBoss/Java EE/Hibernate/OSGi/etc.  This is one of the reasons why we considered setting storeAsBinary to true by default (and we see similar techniques in competing data grids).

This is clear now, thanks. > > Cheers > Manik > > > On 19 May 2011, at 10:55, Galder Zamarreño wrote: > >> would be different cache instances. The problem then is that if an RPC comes for "entities" cache and entity P1, which of the "entities" caches do I go for? You'd need to know which classloader P1 is living in the remote node and you'd have to now that at the Infinispan level to be able to store it in a non-binary format. > > -- > Manik Surtani > manik(a)jboss.org > twitter.com/maniksurtani > > Lead, Infinispan > http://www.infinispan.org > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Galder Zamarreño Sr. Software Engineer Infinispan, JBoss Cache _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

So I think to ever justify it, it would take some very convincing numbers that eager deserialization is indeed better.

On 5/18/11 11:06 AM, Manik Surtani wrote: > > 1) Class loader per session/cache. > > I like Jason/Sanne/Trustin's suggestions of a session-like contract, and specifically I think this is best achieved as a delegate to a cache, again as suggested elsewhere by Pete, etc. E.g., > > Cache<?, ?> myCache = cacheManager.getCache("myCache", myClassLoader); -snip- I would recommend leaving this open to store other per-session configuration values (perhaps with a builder), and some of them mutable. This will allow you to completely eliminate the ThreadLocal context stuff used today which is both faster and more robust (the gc will clean up the state for you).

> 2) Class loader per invocation. > If this "session" notion has some mutable values, you could also make CL mutable: cacheSession.setClassLoader(blah); cacheSession.setFlags(FORCE_WRITE_LOCK)

From an implementation perspective, given where we are with 5.0 now, I suggest we implement by holding the ClassLoader in the CacheDelegate impl, and each method invocation impl would:

> > Yes, no reason why the delegate Cache can't do this. These setters would need to exist on the Cache interface though. For now, we should just restrict to setClassLoader(). > >> From an implementation perspective, given where we are with 5.0 now, I suggest we implement by holding the ClassLoader in the CacheDelegate impl, and each method invocation impl would: > > 1) Set TCCL with the instance's ClassLoader field > 2) Do work > 3) In a finally block, reset TCCL. > > This is just a temp measure, since I don't want to re-work how Marshallers, etc get a hold of the class loader. They use TCCLs right now, and while sub-optimal in many ways, this approach gives us an easy mechanism to implement while still preventing any leaks, etc otherwise common with TCCLs. > The CacheDelegate instance returned by CM.getCache() is shared between all the users that called getCache(), so if we want different users to have different classloaders I think we need another Cache wrapper class that will set/reset the TCCL.

We've decided that rather than swap out the TCCL (which is frankly a bit error prone), to fix this properly and have it so that each time a class is loaded it must select the classloaders it wants. To help, we will add an advancedCache.getClassLoader(), which defaults to the TCCL but can be overridden as described below. As a first step, I have removed the TCCL from the default lookup in Util (AFAICT all class loading was going through there), and required that, if you want to lookup app classes (as opposed to Infinispan classes) you explicitly pass in a classloader. I have then passed in the TCCL as a parameter throughout the codebase. This now makes it explicit where the TCCL is being used and should mean that any new work does think about whether they need to look at app classes or not. Under this new scheme we have 45 refs to the TCCL in the codebase. Some of these are: a) not needed, we are only ever loading system classes b) can, with a bit of a refactor, refer to the classloader we select using withClassLoader() c) other (these will be harder to fix :-() My plan is to go through each one, and chat with the owner of the code and discuss which of (a), (b) or (c) is relevant here.