[Geoserver-devel] GeoServer reload action consequences on catalog listeners

GeoServer GeoServer Developer
1

Hi,
I was trying to debug a seemingly JDK 7 specific issue in the
GWCIntegrationTest and
found a more serious one.
Basically under JDK7 the executiohn order of the test methods inside a
test class
changes dependending on whether I run them from Eclipse, in Maven in a full
build, in maven in a single test build (lots of fun).

Now, there are some parts of the GWCIntegrationTest that do eliminate a layer,
and I already added a revert so that the layer comes back for the
other tests, but
when the first method run happens to be testMissingGeoServerLayerAtStartUp
well, that does not help and most of the other test methods fail.

I've tracked it down to the method calling getGeoServer.reload(), and
by looking into
it something scary came up: when we call that method we loose all
CatalogListener
objects previously attached, as a result of Catalog.disponse() being called.

When that happens we loose significant pieces of functionality:
* the gwc catalog listeners, that synchs up the cache with the
add/remove/modify events
  related to styles and layers
* the namespace/workspace synch listener, making sure the two never
get out of synch
* the update sequence listener

What we don't lose, is the resource pool own cache clearing listener,
probably because
a new resource pool is created in the process.

Looking at the CatalogListener API does not say that the code should
try to reattach
new listeners after GeoServer reload.

Some (possibly ugly) way forwards:
a) change the docs and tell implementors to listen for reload events,
and reattach listeners
b) grab the existing listeners from the catalog, reload, and reattach
them to the
    catalog afterwads (but we'd have to pay attention to skip attaching the old
    resource pool listener)
c) don't get rid of the listeners list in the Catalog.dispose() call
(same issue as b
    with the resource pool one though)

Hum... thoughts on what's the best way to proceed?

Cheers
Andrea

PS: the only sane way to do a) would be to implement
ConfigurationListener.reloaded()
      but that method is now deprecated on trunk? I don't remember a
discussion about
      deprecating it?

--

Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

-------------------------------------------------------

2

Hmmm… nasty :slight_smile:

How about we go with (c) and not remove the listeners, but also add a handleDispose() method to the CatalogListener interface?

On Sun, Nov 4, 2012 at 4:12 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
I was trying to debug a seemingly JDK 7 specific issue in the
GWCIntegrationTest and
found a more serious one.
Basically under JDK7 the executiohn order of the test methods inside a
test class
changes dependending on whether I run them from Eclipse, in Maven in a full
build, in maven in a single test build (lots of fun).

Now, there are some parts of the GWCIntegrationTest that do eliminate a layer,
and I already added a revert so that the layer comes back for the
other tests, but
when the first method run happens to be testMissingGeoServerLayerAtStartUp
well, that does not help and most of the other test methods fail.

I’ve tracked it down to the method calling getGeoServer.reload(), and
by looking into
it something scary came up: when we call that method we loose all
CatalogListener
objects previously attached, as a result of Catalog.disponse() being called.

When that happens we loose significant pieces of functionality:

  • the gwc catalog listeners, that synchs up the cache with the
    add/remove/modify events
    related to styles and layers
  • the namespace/workspace synch listener, making sure the two never
    get out of synch
  • the update sequence listener

What we don’t lose, is the resource pool own cache clearing listener,
probably because
a new resource pool is created in the process.

Looking at the CatalogListener API does not say that the code should
try to reattach
new listeners after GeoServer reload.

Some (possibly ugly) way forwards:
a) change the docs and tell implementors to listen for reload events,
and reattach listeners
b) grab the existing listeners from the catalog, reload, and reattach
them to the
catalog afterwads (but we’d have to pay attention to skip attaching the old
resource pool listener)
c) don’t get rid of the listeners list in the Catalog.dispose() call
(same issue as b
with the resource pool one though)

Hum… thoughts on what’s the best way to proceed?

Cheers
Andrea

PS: the only sane way to do a) would be to implement
ConfigurationListener.reloaded()
but that method is now deprecated on trunk? I don’t remember a
discussion about
deprecating it?

–

Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

LogMeIn Central: Instant, anywhere, Remote PC access and management.
Stay in control, update software, and manage PCs from one command center
Diagnose problems and improve visibility into emerging IT issues
Automate, monitor and manage. Do more in less time with Central
http://p.sf.net/sfu/logmein12331_d2d

Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

3

On Mon, Nov 5, 2012 at 2:40 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hmmm... nasty :slight_smile:

How about we go with (c) and not remove the listeners, but also add a
handleDispose() method to the CatalogListener interface?

And this method would be used... hum... how?

What I think I saw is that the old resource pool gets thrown away, and a new one
gets created, but if we keep a reference to the old listener the old resource
pool won't be garbage collected

Cheers
Andrea

--

Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

-------------------------------------------------------

4

On Mon, Nov 5, 2012 at 6:46 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:

On Mon, Nov 5, 2012 at 2:40 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hmmm… nasty :slight_smile:

How about we go with (c) and not remove the listeners, but also add a
handleDispose() method to the CatalogListener interface?

And this method would be used… hum… how?

What I think I saw is that the old resource pool gets thrown away, and a new one
gets created, but if we keep a reference to the old listener the old resource
pool won’t be garbage collected

Just an idea but the thought was that rather than release the reference to the listener to do cleanup the handleDispose() method could be used if the listener does need to do cleanup. Probably not needed i guess though.

As for the resource pool being thrown away and recreated… the way this happens now is a bit of a side affect so I think worth revisiting. On catalog reload a new catalog is read from disk (and a new object created in memory). The creation is what triggers the creation of the new resource pool and re-registration of the cache clearing listener. Later then the “sync” method on the old Catalog instance is called, which copies over the new resource pool and new listener list, which at this point only includes the cache clearing listener.

So… back to how to fix. I still think not clearing the listener list on disposal is probably simplest. By doing this though in order to avoid duplicate listeners we probably want ResourcePool to first remove any existing listeners. What about adding a method to Catalog to remove listeners by type. So the resource pool constructor would become:

catalog.removeListener(CacheClearingListener.class);
catalog.addListener(new CacheClearingListener());

Another idea would be to use a special interface to mark listeners as “Reloadable”, meaning they will be re-registered on the global reload event. And then on disposals the catalog would only remove those listeners.

Cheers
Andrea

–

Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

5

On Mon, Nov 5, 2012 at 3:29 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

So... back to how to fix. I still think not clearing the listener list on
disposal is probably simplest. By doing this though in order to avoid
duplicate listeners we probably want ResourcePool to first remove any
existing listeners. What about adding a method to Catalog to remove
listeners by type. So the resource pool constructor would become:

  catalog.removeListener(CacheClearingListener.class);
  catalog.addListener(new CacheClearingListener());

I like this one, I'll try to give it a shot

Cheers
Andrea

--

Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

-------------------------------------------------------

6

On Mon, Nov 5, 2012 at 8:53 PM, Andrea Aime <andrea.aime@anonymised.com> wrote:

On Mon, Nov 5, 2012 at 3:29 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

So… back to how to fix. I still think not clearing the listener list on
disposal is probably simplest. By doing this though in order to avoid
duplicate listeners we probably want ResourcePool to first remove any
existing listeners. What about adding a method to Catalog to remove
listeners by type. So the resource pool constructor would become:

catalog.removeListener(CacheClearingListener.class);
catalog.addListener(new CacheClearingListener());

I like this one, I’ll try to give it a shot

Here is a pull request:
https://github.com/geoserver/geoserver/pull/60

I first tried what you proposed, but that did not work because the resource
pool listener is attached to the new catalog, which does not have the old
listener, and similar troubles were present with the geoserver persister.

So I ended up doing a “cleanup, save and reattache” in the portion of the
code handling the catalog synch in the GeoServerLoader instead.
This seems cleaner and better centralized.

Can you review this one?

I’ve also created a jira and marked this bug as a blocker for 2.2 since well…
once you do a reload you’re pretty must toast, significant portions
of GeoServer stop working (no wonder we got all these vague reports
that the embedded GWC is not working, it’s hard to associate a catalog
reload to the fact that GWC integration goes bye bye, especially since
quite likely people will notice the GWC issues long after the reload
has been done).

Cheers
Andrea

–

==
Our support, Your Success! Visit http://opensdi.geo-solutions.it for more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it