Graham, Thanks for the detailed overview... Any chance you can provide some pointers into the changes? Maybe using fisheye... https://fisheye3.atlassian.com/changelog/dspace/modules/dspace-services/branches/simplified-prototype?cs=5379
You know I'm for (1) simplified spring only service manager. (2) jdbc data source delivered by servlet container (3) dropping reflectutils. I'm not so sure about dropping the MBean registration completely, the whole idea was that we could have a container level service manager and services deployed across webapplications. But I assume this is just too crazy an approach to maintain and educate others on its usage. I'm just not convinced we would use a servlet container in this manner and simplification is no doubt more important... My only concern is that the ability I put inplace for addons to deliver their own spring config and have the manager load it (in a SpringDM style manner) be maintained, and I see its still there... :-) What if we put it in the dspace-services trunk, adjust the dspace trunk to use it and use this to get some community testing/feedback? If it fairs well over the next month... then we can do an official release of dspace-services just prior to the dspace 1.7.0 release candidates... Mark On Mon, Oct 4, 2010 at 4:55 PM, Graham Triggs <grahamtri...@gmail.com> wrote: > Hello all, > So, we've been debating the performance / resource usage of DSpace 1.6. This > has just happened to coincide with work that I needed to do to qualify > DSpace 1.6 (and/or 1.7) for our own use. So, for the past week or so, I've > been hammering away at profiling DSpace. This isn't a comprehensive analysis > of the entire codebase, but focusing on the serious issue of stability and > resource leakage. > > This immediately led to finding two issues with the dspace-services code and > it's dependencies: > > 1) The resources in the FinalizableReferenceQueue were leaking (this was > noted before in a Tomcat stack trace posted here) - this could have been > avoided if a newer version of reflectutils had been used, along with the > LifeCycleManager it contains > > 2) A ThreadLocal in the CachingServiceImpl was also leaking - the service > did attempt to clean up in the shutdown, but it would only ever have been > able to clear up the resources of the thread executing the shutdown, not all > the other threads that would have processed requests. > > Investigating the second issue, it turned out that this was being used to > keep track of Request scope caches - as well as potentially leaking > resources (across requests), this highlighted a problem in the service > contract, and a dangerous coupling between the implementation of the > services. > > The Caching service was written to return a Request scoped cache without any > valid Request to bind it to - this was explicitly used in the listeners / > filters to acquire a cache and assign request objects into it before the > request service was called to 'start' a request and bind to the scoped > cache. There were also tests that expected this behaviour. Whilst the > request service will clear a request scoped cache if it's been bound to it, > you can't guarantee that the cache gets bound to a request. > > Worse though, the Request service was depending on the Caching service > tracking request scoped caches against the current thread in order to be > able to track the current request. So, the one responsibility that the > request service should have, it was passing on and hoping that another > service might do the job for it without it being explicitly clear in the > contract. > > If that wasn't bad enough, getCache doesn't actually properly respect the > CacheScope when trying to retrieve a particular cache (it could retrieve a > non-Request scoped cache if one of the same name had already been created, > regardless of the scope parameter). The Caching service also registered an > MBean that was never removed. And, to wrap up my examination of the Caching > service, it didn't close an InputStream that it opened. > > So, onwards - wanting to simplify this code a bit (as we don't need to > support non-Spring IoC containers, at least not yet - and we do need to be > able to understand and maintain this code) - I quickly noticed an issue in > the SpringServiceManager where it returned multiple instances of the same > service name, because it was under the mistaken belief that it needed to > track some of the service names itself, when it was actually getting them > returned from Spring. > > And the ServiceMixinManager was entirely redundant - as long as we can rely > on Spring, we can rely on the container to do everything that the Mixin > manager does, without us having to worry about the complexity of it (and it > is both relatively complex, and a source of our problems earlier as it uses > the FinalizableReferenceQueue). I've actually got a real bugbear about us > using the term 'mixin'. A mixin means something specific - a class that > provides an actual implementation, and is combined with other mixins to > produce other classes [through multiple-inheritance]. When you look at what > is implemented in dspace-services under the term mixin, well we don't > actually have mixins... we have interfaces that are implemented by service > classes. Each service class completely and wholly provide implementations of > the methods defined in all the interfaces they implement. That's a concept > that Java has had for a long time, and is (should be) well understood by all > Java programmers. That doesn't make them mixins. We don't/shouldn't want > 'mixins' (multiple inheritance - there is a reason Java doesn't support it). > And we shouldn't be using a term incorrectly just because it's cool. > > While I'm on a terminology rant, I'm not particularly happy about the > 'interceptors' (ie. RequestInterceptor). They are looking and feeling a lot > more like listeners than interceptors - allowing other services to react to > events taking place (start request / end request), rather than affecting > it's behaviour. I've not renamed them, for the sake of keeping as much of > the existing API intact as I can (whilst clearing out [currently] redundant > or problematic code), but actually it seems that it should really be pushing > a synchronous and immediately executing event notification, so that the > listener registration can occur in a single place rather than duplicating > that functionality anywhere similar concepts may be required. > > OK, less ranting, back to actual issues. Related to the RequestService, the > SessionService lifecycle methods really don't seem to make much sense - > should there be a request in place before you start a session? Are you going > to get a clean session or attach to an existing one? Does end terminate the > session, or simply release it? Add to that, the Threading for expiry of > sessions was entirely problematic, existing on a different cycle to the > container session manager. So, I took the brutal decision to kill those > lifecycle methods - you have to have a request to get a session. > > What else? Well, the initial proposal to bring dspace-services into 1.6 > stated that the MBean registration would be removed. That wasn't the case > for the CachingService, nor was it the case for the Kernel - which actually > registered a UUID based MBean name. Basically, a bit pointless. I made it > properly optional (it will register if you specify a name, but won't if > 'anonymous') - but given the proposal for 1.6, I was considering binning it > entirely. Especially as the MBean registration and de-registration was split > over multiple classes. > > Other problems include some odd switching of config names in the > configurationservice when it tries to do variable replacement - such to the > point I was entirely confused why any of the unit tests would ever have > worked. There were also 9 threading issues, and a few other problems, thrown > up by code inspection. > > So, we have a prototype of a simplified and [hopefully mostly :)] corrected > and cleaned up version of dspace-services here: > > http://scm.dspace.org/svn/repo/modules/dspace-services/branches/simplified-prototype/ > > To recap the changes: > > 1) RequestService is no longer dependent on CachingService, or it's internal > implentation > 2) CachingService registers as a listener to RequestService to manage the > lifecycle of Request scope caches, and only returns request scoped caches > when there is a request > 3) Caches returned are always of the correct scope > 4) The request objects thrown into the cache are no longer there (you can > retrieve them from the RequestService) > 5) The Kernel / manager / initializaer classes have been detangled > 6) MBean registration is completely optional (and should be removed imho) > 7) The ServiceMixinManager is removed as it's entirely unnecessary whilst > Spring is our only IoC container (and there is *no* good reason to > complicate things by supporting combining others at this stage) > 8) reflectutils removed (reflection code no longer necessary - other utility > classes provided by JVM or Spring) - this means that the Sakai Maven > repository can be removed (I've had problems in the past with some artifacts > being retrieved from there) > 9) SessionService simplified, and consistent APIs in place across Request > and Session services. > 10) Multiple threading and resource issues cleaned up > > Aside from having to modify a few tests that relied on the bizarre contracts > that were in place (ie. being able to retrieve request scoped caches without > a valid request), and removing tests related to code that is now gone, > everything passes all of the tests that were already in place. This makes it > pretty much a drop in replacement for the existing dspace-services code - > certainly for the core of 1.6, can't say about other code that may be > depending on dspace-services. > > You'll note that because of this, I haven't been as ambitious as I would > like in ripping this apart. I would love to get rid of the kernel startup, > and just rely on Spring starting up the kernel and service managers - > injecting the bean factory into the managers it instantiates as appropriate. > And pull out things like the interceptors / listeners into events reducing > even more complex code potentially duplicated in services. But I had to > start somewhere, and this seems to be the right sort of area for looking at > DSpace 1.7. This shouldn't stop us being more aggressive at simplifying and > reducing this code later. > > And to take us back to the earlier issues about leakage inside a container - > dspace-services alone doesn't make the applications cleanup correctly. There > needs to be some adjustments to the context listeners, and their ordering in > the web.xml. You also have to take care with the JDBC drivers (I now have a > reworked DatabaseManager that allows you to retrieve a DataSource from JNDI > if available, and if not creates a DataSource using the entries in > dspace.cfg, the structure of which means that you do not need the JDBC > driver, DBCP or commons-pool in the webapp if you have a JNDI datasource. > > But with a bit of care and attention taken in the application assembly and > deployment, the JSPUI does unload cleanly. > > XMLUI is still proving a bit more troublesome. There are two instances of > resource leakage within Cocoon itself - in the sitemap code, it puts a stack > into a ThreadLocal, but never resets the contents (even when it's empty). > The flowscript code also doesn't exit the Rhino contexts correctly. Both of > the issues will prevent cleaning up of the resources. > > With patched versions of those jars in place, as well as the same JDBC and > listener issues from jspui - well, it still doesn't quite work reliably > (although the profiler can no longer detect any GC roots). If I disable all > of the sitemap and config reloading, *and* wait 10 minutes after undeploying > the application, it will clean up and unload correctly. All I know right now > is that there isn't a Thread showing up that would prevent this. It looks > like it's some system cache (like in URL keep alives), or a session holding > data. > > But it neither will unload either way with the current version of > dspace-services - it needs the cleaned up/simplified version. > > One more thing about Tomcat - there is often a lot of bad press for Tomcat, > blaming it for not unloading classes. Not only is this blatantly untrue (if > your application can be unloaded, it will be - it's only the application > leaking resources that stops it), they've made a lot of strides in the most > recent releases (6.0.28 / 6.0.29) to help combat potential resources leaks > in your applications. Take a look at the configuration options on Context: > > http://tomcat.apache.org/tomcat-6.0-doc/config/context.html (clear > ThreadLocals / stop threads and timers) > > and the listeners: > > http://tomcat.apache.org/tomcat-6.0-doc/config/listeners.html (JRE leak > protection, that protects against a number of things, including the URL > caching that is problematic on Windows) > > I wouldn't rely on these being able to prevent your application from causing > a problem, and it's much better to ensure that applications are properly > behaved even without Tomcat attempting to prevent problems - but it can > still be useful in some circumstances. > > G > > ------------------------------------------------------------------------------ > Virtualization is moving to the mainstream and overtaking non-virtualized > environment for deploying applications. Does it make network security > easier or more difficult to achieve? Read this whitepaper to separate the > two and get a better understanding. > http://p.sf.net/sfu/hp-phase2-d2d > _______________________________________________ > DSpace-tech mailing list > DSpace-tech@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/dspace-tech > > -- Mark R. Diggory Head of U.S. Operations - @mire http://www.atmire.com - Institutional Repository Solutions http://www.togather.eu - Before getting together, get t...@ther ------------------------------------------------------------------------------ Beautiful is writing same markup. Internet Explorer 9 supports standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3. Spend less time writing and rewriting code and more time creating great experiences on the web. Be a part of the beta today. http://p.sf.net/sfu/beautyoftheweb _______________________________________________ DSpace-tech mailing list DSpace-tech@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dspace-tech
