GeoServer
  1. GeoServer
  2. GEOS-4425

GeoServerLoader reload forgets to load initializers

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1-RC2
    • Fix Version/s: 2.1-RC5
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      2

      Description

      The GeoServerLoader reload method actually reloads only the catalog and geoserver metadata

      loadCatalog( catalog, xp );
      loadGeoServer( geoserver, xp);

      It's should also invoke the

      loadInitializers(geoserver);

      method in order to reload the JAI tilecache and other initializers also.

      1. GEOS-4425.patch
        5 kB
        Andrea Aime
      2. GEOS-4425-2.patch
        4 kB
        Andrea Aime

        Issue Links

          Activity

          Andrea Aime made changes -
          Field Original Value New Value
          Fix Version/s 2.1.0 [ 17175 ]
          Affects Version/s 2.1-RC2 [ 17077 ]
          Priority Major [ 3 ] Critical [ 2 ]
          Hide
          Andrea Aime added a comment -

          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...

          Show
          Andrea Aime added a comment - 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...
          Hide
          Andrea Aime added a comment -

          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

          Show
          Andrea Aime added a comment - 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
          Andrea Aime made changes -
          Attachment GEOS-4425.patch [ 54326 ]
          Hide
          Andrea Aime added a comment -

          The thread pool -> the one used my multithreaded mosaics

          Show
          Andrea Aime added a comment - The thread pool -> the one used my multithreaded mosaics
          Hide
          Justin Deoliveira added a comment -

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

          Show
          Justin Deoliveira added a comment - Looks great Andrea. And yeah I say let's get it in for 2.1-RC3.
          Hide
          Andrea Aime added a comment -

          Fixed on 2.1.x and trunk

          Show
          Andrea Aime added a comment - Fixed on 2.1.x and trunk
          Andrea Aime made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Justin Deoliveira made changes -
          Fix Version/s 2.1-RC3 [ 17257 ]
          Fix Version/s 2.1.0 [ 17175 ]
          Hide
          Andrea Aime added a comment -

          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

          Show
          Andrea Aime added a comment - 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
          Andrea Aime made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Andrea Aime added a comment -

          Ah hem... Justin?

          Show
          Andrea Aime added a comment - Ah hem... Justin?
          Hide
          Justin Deoliveira added a comment -

          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

          Show
          Justin Deoliveira added a comment - 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
          Hide
          Andrea Aime added a comment -

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

          Show
          Andrea Aime added a comment - Ok. Humm... so we modify the API now? This issue is making the status page useless after a reload, people keep on reporting it...
          Andrea Aime made changes -
          Link This issue relates to GEOS-4468 [ GEOS-4468 ]
          Andrea Aime made changes -
          Link This issue relates to GEOS-4481 [ GEOS-4481 ]
          Hide
          Zsolt Sandor added a comment -

          We can still reproduce it in 2.1-RC4

          Show
          Zsolt Sandor added a comment - We can still reproduce it in 2.1-RC4
          Hide
          Justin Deoliveira added a comment -

          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.

          Show
          Justin Deoliveira added a comment - 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.
          Andrea Aime made changes -
          Fix Version/s 2.1.0 [ 17175 ]
          Fix Version/s 2.1-RC3 [ 17257 ]
          Hide
          Andrea Aime added a comment -

          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?

          Show
          Andrea Aime added a comment - 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?
          Hide
          Andrea Aime added a comment -

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

          Show
          Andrea Aime added a comment - Here is the patch removing the cleaning of config listeners ( GEOS-4425 -2.patch)
          Andrea Aime made changes -
          Attachment GEOS-4425-2.patch [ 54695 ]
          Andrea Aime made changes -
          Attachment GEOS-4425-2.patch [ 54696 ]
          Andrea Aime made changes -
          Attachment GEOS-4425-2.patch [ 54695 ]
          Hide
          Andrea Aime added a comment -

          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.

          Show
          Andrea Aime added a comment - 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.
          Hide
          Justin Deoliveira added a comment -

          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.

          Show
          Justin Deoliveira added a comment - 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.
          Hide
          Andrea Aime added a comment -

          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?

          Show
          Andrea Aime added a comment - 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?
          Hide
          Justin Deoliveira added a comment -

          Yup, that looks good.

          Show
          Justin Deoliveira added a comment - Yup, that looks good.
          Hide
          Andrea Aime added a comment -

          Fixed on 2.1.x and trunk

          Show
          Andrea Aime added a comment - Fixed on 2.1.x and trunk
          Andrea Aime made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Andrea Aime made changes -
          Fix Version/s 2.1-RC5 [ 17324 ]
          Fix Version/s 2.1.0 [ 17175 ]
          Hide
          Andrea Aime added a comment -

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

          Show
          Andrea Aime added a comment - Mass closing all issues that have been in "resolved" state for more than a month without further comments
          Andrea Aime made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Andrea Aime
              Reporter:
              Alessio Fabiani
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: