Ok, so after some discussion with Thomas: * I agree that solution 2.3 is nice and we should probably go towards it in the future * I need a quick solution that can be committed for 6.2.1
As a conclusion I’ll implement the @DisposePriority annotation for now (it’s simple to do, I have it done actually). Then in the future we’ll be able to either keep it or deprecate it in favor of a better solution. Thanks -Vincent On 24 Sep 2014 at 09:46:38, Thomas Mortagne ([email protected](mailto:[email protected])) wrote: > On Wed, Sep 24, 2014 at 9:23 AM, [email protected] wrote: > > Hi Thomas, > > > > On 24 Sep 2014 at 08:58:56, Thomas Mortagne > > ([email protected](mailto:[email protected])) wrote: > > > >> On Tue, Sep 23, 2014 at 11:48 PM, [email protected] wrote: > >> > Hi devs, > >> > > >> > Right now there’s a problem when XWiki is shutdown. This is what > >> > currently happens: > >> > > >> > 2014-09-23 13:54:05,563 [Thread-1] DEBUG o.x.shutdown - Starting XWiki > >> > shutdown... > >> > 2014-09-23 13:54:05,563 [Thread-1] DEBUG o.x.shutdown - Stopping > >> > Database Connection Pool... > >> > 2014-09-23 13:54:05,735 [Thread-1] DEBUG o.x.shutdown - Database > >> > Connection Pool has been stopped > >> > 2014-09-23 13:54:10,513 [Thread-1] DEBUG o.x.shutdown - Stopping > >> > component > >> > [org.xwiki.localization.wiki.internal.DocumentTranslationBundleFactory]… > >> > 2014-09-23 13:54:10,513 [Thread-1] DEBUG o.x.shutdown - Component > >> > [org.xwiki.localization.wiki.internal.DocumentTranslationBundleFactory] > >> > has been stopped > >> > … > >> > 2014-09-23 13:54:10,514 [Thread-1] DEBUG o.x.shutdown - Stopping > >> > component [org.xwiki.search.solr.internal.DefaultSolrIndexer]... > >> > 2014-09-23 13:54:10,514 [Thread-1] DEBUG o.x.shutdown - Component > >> > [org.xwiki.search.solr.internal.DefaultSolrIndexer] has been stopped > >> > … > >> > 2014-09-23 13:54:10,539 [Thread-1] INFO o.x.shutdown - XWiki shutdown > >> > has been completed. > >> > > >> > As you can see the problem is that the DB is stopped first. So any code > >> > accessing the DB will get an error once the connection pool is stopped. > >> > This is why we get a lot stack traces when you ctrl-c XWiki quickly > >> > after it starts (i.e. when the init is not finished or when SOLR is > >> > still indexing). You can also get stack traces if a watchlist is being > >> > sent or some scheduler job is executing for example. > >> > > >> > There are 2 ways to fix this: > >> > - solution 1: all code that accesses the DB could check if an XWiki > >> > shutdown is in progress and don’t generate stack traces if this is the > >> > case > >> > - solution 2: stop the DB as the last thing, after everything else has > >> > stopped > >> > > >> > Obviously solution 2 is much better. > >> > > >> > I’ve started implementing it by doing this: > >> > - Remove the current HibernateShutdownEventListener since this is > >> > executed first during shutdown, before component disposal > >> > - Instead have XWikiHibernateStore implement Disposable and move the > >> > content of HibernateShutdownEventListener in that method > >> > > >> > This works already better but it’s not enough since ECM.dispose() works > >> > out the component dependencies but this works only for explicit > >> > dependencies and doesn’t work if a component uses the ComponentManager > >> > to dynamically find implementations (as this is the case for example for > >> > DefaultSolrIndexer). > >> > > >> > Thus in order to help the component disposal process, I propose to > >> > introduce a new annotation: @DisposePriority(int). > >> > > >> > Note that an alternative would be to introduce a new Disposable > >> > interface with a new getPriority() method in it (we cannot modify the > >> > current Disposable since that would break backward compat). > >> > >> It's always hard to know what priority to put exactly so this is last > >> resort solution for me. > > > > The idea is to use this only for special cases (like DB shutdown) since in > > the majority of cases the component dependency order would be enough. So in > > these few cases it’s quite easy and similar to Macro priority (which hasn’t > > proved to be a problem so far and is working well). > > > >> A few other alternatives: > >> 1) stop the DB after disposing components (I would not vote for it I > >> think but it should work for this use case) > > > > Yes I thought about this but I wanted something a bit more generic. See at > > the end of this email though. > > > >> 2) introduce something like a @ComponentDependency annotation to let > >> component declare indirect dependencies (ECM#dispose would simply add > >> @ComponentDependency annotations to its current dependencies based > >> ordering) and: > >> 2.1) even when a component does not directly use XWikiStoreInterface > >> (pretty much all of them are using XWiki methods and don't even know > >> about it officially) we could still ask them to declare something like > >> @ComponentDependency(XWikiStoreInterface.class) to indicate that they > >> want to be disposed after any component of this role. > > > > Yes I had also thought about this one but discarded because it means > > finding all components using indirectly XWikiStoreInterface and modifying > > them all. This means a lot of places (since a lot of places use the XWiki > > class for example) and a lot of work. > > > > I also find it a lot more complex to implement than a priority order which > > seems really the most simple solution. > > The dependencies based ordering is already what we have and 2) is just > about adding more depdencies. > > The first need is to add the annotation in the few places where we > have the issue (only SOLR right now AFAIK) so no it does not really > mean "finding all components using indirectly XWikiStoreInterface and > modifying them all" right away. > > > > >> 2.2) make XWiki instance a component instead of storing it in the > >> servlet context and inject it instead of accessing it through > >> XWikiContext (XWiki itself would inject the component it want). XWiki > >> itself would declare a > >> @ComponentDependency(XWikiStoreInterface.class). > >> 2.3) move all storage related methods from XWiki (saveDocument, > >> getDocument, etc) to another component (and remove XWikiContext > >> parameter to all these methods) which would be injected by components > >> directly instead of searching for the XWikiContext (that's how new > >> model will work but we can have another one before that still expose > >> olcore APIs like XWikiDocument). Then this component would take care > >> of declaring the @ComponentDependency(XWikiStoreInterface.class) (it's > >> not really possible to directly inject the right XWikiStoreInterface > >> since it's configuration based) > > > > These 2 alternatives are ok but they are a lot of work and could be done in > > the future. I was trying to find a solution that can be applied in 1/2 day > > right now. We need to fix our bug now and I fear we’ll just not do anything > > if we wait for 2.2 and 2.3 (which we can do on the longer run). > > 2.3 is mostly copy pasting and start using the new component in the > few places that have ordering issue (pretty much only SOLR right now). > Changing other places is not a priority, it's not like we were > refactoring the whole code each time we introduce a new API. > > > > > Since you don’t agree about priority and since I’d like to fix the shutdown > > “bug” for 6.2.x and on 6.3M1 now, I’m proposing the following alternative > > for now: > > > > * Introduce a new constructor in the ApplicationStoppedEvent class: > > ApplicationStoppedEvent(boolean afterDispose) > > * Modify XWikiServletContextListener.contextDestroyed() so that it sends > > the ApplicationStoppedEvent(false) before calling cm.dispose() and it sends > > the ApplicationStoppedEvent(true) after calling cm.dispose() > > * Keep HibernateShutdownEventListener but make it listen to > > ApplicationStoppedEvent(true) > > > > This will ensure it’s called after all components have been disposed. > > I doubt that would work. If the CM is disposed > HibernateShutdownEventListener won't exist anymore (and I'm not > talking about using the observation manager which is going to be > disposed officially, it's not doing anything special when disposed > right now but it might). > > > > > WDYT? Acceptable? > > > > Thanks > > -Vincent > > > >> > The idea is then to use this information in ECM.dispose() to first sort > >> > all components based on this and then only to order them with their > >> > dependencies. > >> > > >> > WDYT? > >> > > >> > Thanks > >> > -Vincent > > _______________________________________________ > > devs mailing list > > [email protected] > > http://lists.xwiki.org/mailman/listinfo/devs > > > > -- > Thomas Mortagne > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
