GeoServerLoader reload forgets to load initializers

Hmmm... I agree we should also call back initializers... on the other side all the initializers we have around are coded to be called just once, and add listeners to be informed of further changes (so calling them every time we reload would actually introduce a small memory leak).

I'm under the impression the problem is not exactly that the initializers are not called, but the initializer listeners are not reacting to the change.
I did look and the event triggered past the reload is a ConfigurationListener.handlePostGlobalChange, whilst the listeners in ResourcePool and JAIInitializer are listening for handleGlobalChange instead and thus do not reload their stuff.

Solutions I have in mind:

• have GeoSErverImpl also trigger a fireGlobalModified()
• have the jai and resource pool initializers listen also to handlePostGlobalChange (doing the same work)

I think I'd go for the second. Justin, what do you think?
This is something that should go in RC3...

Here is a patch for this one. Adds the usage of the handlePostGlobalChange in the listeners, and also moves the initialization of the thread pool from the resource pool initializer to coverage access initializer.
The latter is for consistency, CoverageAccessInitializer actually configures the thread pool, ResourcePoolInitializer set is into the resource pool, but if they were executed out of order the resource pool could have been left with an invalid or misconfigured pool

The thread pool -> the one used my multithreaded mosaics

Looks great Andrea. And yeah I say let's get it in for 2.1-RC3.

Fixed on 2.1.x and trunk

Unfortunately this one is not really fixed.. I thought so but missed a bit.

When the GeoServerLoader does the reload it calls destroy on GeoServerImpl, that in turn clears up all the listeners.
This means the listeners setup by the initializers are lost, and since the initializer are called just once, they are lost forever.
This is btw also the cause for the problem reported on the user list about the status page going NPE, it's due to the jai initializer code not rerunning and not setting up jai properly (which results in some null objects around, thus the NPE).

Now, I have in mind two solutions for this one:

• go back and apply the solution originally suggested by Alessio. That one is simple, but some initializers will have to be re-coded to know they can be called multiple times (but that they have to reattach listeners... might get a bit convoluted)
• make a copy of the listeners list before destroy, and set it back. Tried this one and it works, it also makes sense from the p.o.v of the initializers, but unfortunately it also results in a leak, because there are other parts that are reattaching listeners, so the list grows, first 9 listeners, reload, 11, reload, 13 and so on

Justin, opinions on this one? I guess the path chosen by Alessio works better, but that will require changes in the way some services (monitoring afaik) do their initialization

Ah hem... Justin?

Yeah I agree that the initial approach seems to be the best one. And yeah... there are quite a few initializers around so it would be good to do a quick scan and figure out which ones need to be updated. And yeah... the monitoring initializer is a bit of an issue as it assumes only being called once and would actually corrupt data if called during a reload.

Sounds like we need to add explicit life cycle methods to the GeoServerInitializer interface:

initialize() -> called once on start up
reload() -> called on reload
destroy() -> called on shut down

Ok. Humm... so we modify the API now?
This issue is making the status page useless after a reload, people keep on reporting it...

We can still reproduce it in 2.1-RC4

I agreed that the approach you presented (Alessio's initial approach) is fine. But as a (possibly future) additional improvement it seems like that interface requires explicit life cycle methods. I leave it to you whether to do the api change now or leave it for later.

Still struggling to fix this one.
Ok, summary of the facts that I've collected so far:

• we already have a lifecycle interface that tells about reset, dispose and reload, it's called GeoServerLifecycleHandler (so if anything we should probably kill the initializers and add the initiliaze() method to the GeoServerLifecycleHandler?)
• the initializers are supposed to be called only once, yet the listeners they attach to the system get wiped out on reload.

The second is actually the real problem. There is many pieces of code that attach listeners to the configuration just once, we cannot just wipe them out on reload. Here is a list:

• most initializers attach listeners
• the update sequence listener, which is attached just once on spring bean loading
• the WFSConfiguration internal lister (same as above)
• the QuickTileCache one (same as above)

So it appears that clearing the listers is not a good idea... I cooked up a patch that keeps the listeners intact, and that makes sure they don't accumulate. The only piece of code I found that was adding listers repeatedly during reloads was the DefaultGeoServerLoader, which I modified to attach just one listener instead, see attached patch.
Also made a super-complete build including community release, the only module failing is the HibCatalogConfigImplTest one (when run in Maven, that also runs the superclass tests), but for the life of me I can't see why, I don't see anything in the hibernate configuration code doing either a config listener attach, or using the DefaultGeoServerLoader

Some help here?

Here is the patch removing the cleaning of config listeners (GEOS-4425-2.patch)

Ah, never mind, it seems the hibernate config module fails the build regardless of my patch, it's just that the nightly build does not run tests so it does not get noticed.

Patch looks good although one question. I notice there is a change to attach the configuration persister before readConfiguration is called, as opposed to after. Is this intended? I guess because we only want to add it once and not on subsequent reloads. The reason (iirc) we do it after is because during the population of the catalog we don't want to have a save/serialization triggered for every object added. The only time we do is when migrating from an older data directory.

So perhaps the configuration persister should also be stored as a member variable and checked in the same way after the call to readConfiguration()? And actually it should probably be removed before the call to prevent what I described above.

See, was not aware of this, good thing we have reviews in place.

So the old code was relying on the listener to be removed automatically and added it back afterwards.
If I follow what you say the current behavior should be fixed so that persister is not added along with the other listener, but we have something like:

   try {
      if(this.persister != null) {
         // avoid having the persister write down new config files while we read the config
         geoserver.removeListener(persister);
      } else {
         this.persister = new GeoServerPersister( resourceLoader, xp );
      }
      readConfiguration(geoServer, xp);
   } finally {
      geoserver.addListener(persister);
   }

Does that look ok?

Yup, that looks good.

Fixed on 2.1.x and trunk

Mass closing all issues that have been in "resolved" state for more than a month without further comments