[Geoserver-devel] Serious issue found with Wicket front-end

GeoServer GeoServer Developer
1

Hi,
lately I’ve been spending some time trying to figure out why users keep on reporting,
every now and then, about CatalogInfo subclasses throwing NullPointerException
related to the catalog field in them being null.

Now, generally speaking, we know two things about that field:

  • it gets populated as we create new objects
  • the field is however not serializable

This means that if a CatalogInfo object holding a reference to the catalog gets serialized
and then deserialized, it won’t have a reference to the catalog anymore.
We do compensate for that in some places, like in LayerModel for example, calling
CatalogBuilder.attach(LayerInfo).

However, there are several places which are not doing that, and keeping tabs with
everything, with a moving target such as GeoServer, with new people going in
all the time, it’s not a winning move (imho).

Actually I’ve found a simple way to replicate one of the NPE issues:

  • open topp:states
  • in a separate tab, go to layers, new, republish topp:states as topp:states2,
    but don’t save
  • go back into topp:states, change tabs a bit, save
  • go back to topp:states2, save
  • go previewing topp:states2, ka-blam, NPE while the StyleInfo attached to that
    layer tries to fetch the SLD

Why is this happening? It happens because the Wicket state is managed server side,
and if you are trying to use the same session with separate pages it will eventually
serialize the state of the other page in disk.
When it needs it, it will deserialize it, and that’s where the problem happens,
as the de-serialized CatalogInfo objects will miss their reference to Catalog.
They get saved that way, and when you try to use them, boom.

Now, the GUI never uses the CatalogInfo directly, is has references through the ModificationProxy,
so I’ve tried a simple patch to reattach them as they are de-serialized, by adding a readResolve
to ModificationProxy (see attachment).

That seems to do the trick, no more NPE, however I’m not so sure it’s the real solution.
The thing is, the de-serialized object is still a copy of the CatalogInfo object the catalog
is maintaining, the information has been duplicated: what I’m concerned about is that,
modifying one, the other will not be touched.

An alternative solution may be not to restore the catalog reference, but to replace
the proxied object (the proxyObject field in ModificationProxy) with a reference to
the actual bean managed by the catalog.
This would require us to save in ModificationProxy not only the proxyObject, but also
the interface it was proxied for, so that we can call:
public T get(Class type, Filter filter)
with, I guess, a filter matching the id of the object that we serialized.

Opinions?

This is something I’d really like to have fixed by 2.3, but it’s not the end of the world
if we don’t: after all, this bug has been in GeoServer since 2.0, that is, for well over 2 years.

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

(attachments)

mp-reattach.patch (2.27 KB)

2

Hmmm… nasty problem.

I do think I like the approach of having the proxy reinitialize itself post serialization grabbing the real object is a bit cleaner long term but this approach is also simpler which is nice. No strong opinion.

What sort of hooks do we get into the wicket serialization/deserialization objects? We did talk about one pointing making the CatalogBuilder.attach methods part of the core catalog api. If we had sufficient hooks into the serialization we could use that api to “reattach”… although i am not sure how much that buys us.

Sorry… nothing very useful to add :slight_smile:

···

On Sun, Jan 27, 2013 at 7:52 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
lately I’ve been spending some time trying to figure out why users keep on reporting,
every now and then, about CatalogInfo subclasses throwing NullPointerException
related to the catalog field in them being null.

Now, generally speaking, we know two things about that field:

  • it gets populated as we create new objects
  • the field is however not serializable

This means that if a CatalogInfo object holding a reference to the catalog gets serialized
and then deserialized, it won’t have a reference to the catalog anymore.
We do compensate for that in some places, like in LayerModel for example, calling
CatalogBuilder.attach(LayerInfo).

However, there are several places which are not doing that, and keeping tabs with
everything, with a moving target such as GeoServer, with new people going in
all the time, it’s not a winning move (imho).

Actually I’ve found a simple way to replicate one of the NPE issues:

  • open topp:states
  • in a separate tab, go to layers, new, republish topp:states as topp:states2,
    but don’t save
  • go back into topp:states, change tabs a bit, save
  • go back to topp:states2, save
  • go previewing topp:states2, ka-blam, NPE while the StyleInfo attached to that
    layer tries to fetch the SLD

Why is this happening? It happens because the Wicket state is managed server side,
and if you are trying to use the same session with separate pages it will eventually
serialize the state of the other page in disk.
When it needs it, it will deserialize it, and that’s where the problem happens,
as the de-serialized CatalogInfo objects will miss their reference to Catalog.
They get saved that way, and when you try to use them, boom.

Now, the GUI never uses the CatalogInfo directly, is has references through the ModificationProxy,
so I’ve tried a simple patch to reattach them as they are de-serialized, by adding a readResolve
to ModificationProxy (see attachment).

That seems to do the trick, no more NPE, however I’m not so sure it’s the real solution.
The thing is, the de-serialized object is still a copy of the CatalogInfo object the catalog
is maintaining, the information has been duplicated: what I’m concerned about is that,
modifying one, the other will not be touched.

An alternative solution may be not to restore the catalog reference, but to replace
the proxied object (the proxyObject field in ModificationProxy) with a reference to
the actual bean managed by the catalog.
This would require us to save in ModificationProxy not only the proxyObject, but also
the interface it was proxied for, so that we can call:
public T get(Class type, Filter filter)
with, I guess, a filter matching the id of the object that we serialized.

Opinions?

This is something I’d really like to have fixed by 2.3, but it’s not the end of the world
if we don’t: after all, this bug has been in GeoServer since 2.0, that is, for well over 2 years.

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

Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only – learn more at:
http://p.sf.net/sfu/learnnow-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 Tue, Jan 29, 2013 at 1:02 AM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hmmm… nasty problem.

I do think I like the approach of having the proxy reinitialize itself post serialization grabbing the real object is a bit cleaner long term but this approach is also simpler which is nice. No strong opinion.

What sort of hooks do we get into the wicket serialization/deserialization objects? We did talk about one pointing making the CatalogBuilder.attach methods part of the core catalog api. If we had sufficient hooks into the serialization we could use that api to “reattach”… although i am not sure how much that buys us.

Yes, looked into this one too, unfortunately we need a more recent version of Wicket to be able to roll our own
custom serializer (I believe 1.5.x, and 1.6.x provides ever better hooks).

As long as we are on 1.4.x this is a no go, the serializer is pretty much hard coded as far as I could see.

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 Tue, Jan 29, 2013 at 1:02 AM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hmmm… nasty problem.

I do think I like the approach of having the proxy reinitialize itself post serialization grabbing the real object is a bit cleaner long term but this approach is also simpler which is nice. No strong opinion.

Hi,
a week ago I’ve committed a change that fixes the issue by reattaching the current in memory live object(s)
into the modification proxy (and prepared this mail too, but then forgot to send it… grrr…):
https://github.com/geoserver/geoserver/commit/db338a6ea1e86371c24f7e8b827e33f9bb24cee8

I’ve tried to go as deep as possible, but the code is still prone to issues in particular cases.
In particular, CatalogInfo objects being reattached are:

  • the proxyObject field
  • any CatalogInfo contained in the dirty properties of the modification proxy, or contained
    in a collection or MetadataMap inside the dirty properties
  • any CatalogInfo in a collection or MetadataMap inside the old collection values

I did not go any further than that. Of course, theoretically, a dirty property could contain
an object that is not a CatalogInfo, but that references one.
That could be handled by doing a breath first search in the fields of all beans involved
in the serialization using reflection, but it would not be trivial (e.g., the beans could
contain loops, or have their own readResolve that attaches them to a large object
network in memory).
Given that today we don’t seem to have this case (non CatalogInfo object referring
to a CatalogInfo one) I let that go…

Can somebody review?

How about backporting to 2.3.x? The bug in question is rather annoying and
has caused a lot of pain (managed to find several reports in Jira that can
be tracked down to this one)

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

5

The approach I would like the better, which would require a GSIP, but
throwing it out here for discussion, is to get rid of Catalog as an
instance attribute of the configuration objects, and also to remove
the ModificationProxy altogether, making *Info instances true data
objects, serializable and clonable, and holding only its own state.

This imho would lead to an "easier place where to live" for
developers, with a clean separation from configuration and business
objects.
On the Catalog being referenced by *StoreInfo and maybe other Info
objects, it is solely in order to provide access to the catalog's
ResourcePool inside DataStoreInfo.getDataStore() for example. Now,
whenever you have a handle on a DataStoreInfo (data object) and need
access to the DataStore (business object) you most probably have other
means to access the Catalog/ResourcePool and query it directly with
your DataStoreInfo or perhaps better with just its id.

On the ModificationProxy proposal, my (rather limited) understanding
is it provides for the list of changed properties for catalog events.
Now, IMHO, the added complexity (ModificationProxy is a rather complex
beast itself) is not worth it compared to a design where *Info objects
are just POJOS that can be cloned/serialized with no extra care, where
the (default, in-memory) Catalog return clones of the queried objects
instead of proxies, and where when the changed properties need to be
figured out (like in for throwing catalog events) the two POJO
instances (the current and the one being modified) are run through
some sort of diff function that returns the list of changed
properties.

All of these would hopefully lead to doing more (or the same) with
less code to maintain. Hopefully just that "diff" kind of function,
and making sure *Info classes have proper clone() methods, which would
probably make the life of new and old developers easier as IMHO this
would be much easier to understand by following "traditional" or
"standard" java practices?

This also brings some other little flaws like JAIInfo.getJAI()
returning the default JAI instance and setJAI being of no value,
making me think JAI should not be part of the configuration model but
obtained somehow else using the configuration given by JAIInfo.

Another one would be I also don't understand why Catalog extends
CatalogInfo, guess it'd be time to stop doing so.

My 2 cents,
Gabriel

On Mon, Jan 28, 2013 at 9:02 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hmmm... nasty problem.

I do think I like the approach of having the proxy reinitialize itself post
serialization grabbing the real object is a bit cleaner long term but this
approach is also simpler which is nice. No strong opinion.

What sort of hooks do we get into the wicket serialization/deserialization
objects? We did talk about one pointing making the CatalogBuilder.attach
methods part of the core catalog api. If we had sufficient hooks into the
serialization we could use that api to "reattach"... although i am not sure
how much that buys us.

Sorry... nothing very useful to add :slight_smile:

On Sun, Jan 27, 2013 at 7:52 AM, Andrea Aime <andrea.aime@anonymised.com>
wrote:

Hi,
lately I've been spending some time trying to figure out why users keep on
reporting,
every now and then, about CatalogInfo subclasses throwing
NullPointerException
related to the catalog field in them being null.

Now, generally speaking, we know two things about that field:
- it gets populated as we create new objects
- the field is however not serializable

This means that if a CatalogInfo object holding a reference to the catalog
gets serialized
and then deserialized, it won't have a reference to the catalog anymore.
We do compensate for that in some places, like in LayerModel for example,
calling
CatalogBuilder.attach(LayerInfo).

However, there are several places which are not doing that, and keeping
tabs with
everything, with a moving target such as GeoServer, with new people going
in
all the time, it's not a winning move (imho).

Actually I've found a simple way to replicate one of the NPE issues:
- open topp:states
- in a separate tab, go to layers, new, republish topp:states as
topp:states2,
  but don't save
- go back into topp:states, change tabs a bit, save
- go back to topp:states2, save
- go previewing topp:states2, ka-blam, NPE while the StyleInfo attached to
that
  layer tries to fetch the SLD

Why is this happening? It happens because the Wicket state is managed
server side,
and if you are trying to use the same session with separate pages it will
eventually
serialize the state of the other page in disk.
When it needs it, it will deserialize it, and that's where the problem
happens,
as the de-serialized CatalogInfo objects will miss their reference to
Catalog.
They get saved that way, and when you try to use them, boom.

Now, the GUI never uses the CatalogInfo directly, is has references
through the ModificationProxy,
so I've tried a simple patch to reattach them as they are de-serialized,
by adding a readResolve
to ModificationProxy (see attachment).

That seems to do the trick, no more NPE, however I'm not so sure it's the
real solution.
The thing is, the de-serialized object is still a copy of the CatalogInfo
object the catalog
is maintaining, the information has been duplicated: what I'm concerned
about is that,
modifying one, the other will not be touched.

An alternative solution may be not to restore the catalog reference, but
to replace
the proxied object (the proxyObject field in ModificationProxy) with a
reference to
the actual bean managed by the catalog.
This would require us to save in ModificationProxy not only the
proxyObject, but also
the interface it was proxied for, so that we can call:
public <T extends CatalogInfo> T get(Class<T> type, Filter filter)
with, I guess, a filter matching the id of the object that we serialized.

Opinions?

This is something I'd really like to have fixed by 2.3, but it's not the
end of the world
if we don't: after all, this bug has been in GeoServer since 2.0, that is,
for well over 2 years.

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

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

------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnnow-d2d
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

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

------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnnow-d2d
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

6

On Mon, Feb 11, 2013 at 8:08 PM, Gabriel Roldan <groldan@anonymised.com> wrote:

This imho would lead to an “easier place where to live” for
developers, with a clean separation from configuration and business
objects.
On the Catalog being referenced by *StoreInfo and maybe other Info
objects, it is solely in order to provide access to the catalog’s
ResourcePool inside DataStoreInfo.getDataStore() for example. Now,
whenever you have a handle on a DataStoreInfo (data object) and need
access to the DataStore (business object) you most probably have other
means to access the Catalog/ResourcePool and query it directly with
your DataStoreInfo or perhaps better with just its id.

At the moment the ResourcePool is “hidden”, there is no
direct usage in the services code, and *Info objects are passed around
in many places that have no direct access to the Catalog (as in, they lack
direct access to the Spring context, though that might be dodged using
GeoServerExtensions).

On the ModificationProxy proposal, my (rather limited) understanding
is it provides for the list of changed properties for catalog events.
Now, IMHO, the added complexity (ModificationProxy is a rather complex
beast itself) is not worth it compared to a design where *Info objects
are just POJOS that can be cloned/serialized with no extra care, where
the (default, in-memory) Catalog return clones of the queried objects
instead of proxies, and where when the changed properties need to be
figured out (like in for throwing catalog events) the two POJO
instances (the current and the one being modified) are run through
some sort of diff function that returns the list of changed
properties.

The approach as described is quite problematic. Here is an example:

  • Info object A is cloned by thread1, making B. Properties p1 and p2 get modified
  • Info object A is cloned in thread2, making C. Property p3 is modified
  • Thread1 saves, a comparison against the in-catalog copy A is made and p1 and p2
    get modified in A too
  • Thread2 saves, a comparisong against the in-catalog copy A is made and p3 is
    saved, p1 and p2 are reverted to their previous state

The above mishbehavior cannot happen using ModificationProxy, as the original
state of the bean is carried around.
While the solution is not the same, the behavior is like Hibernate one, where the
original state of the bean is kept in dehidrated form in the Session (that is, in a thread
local)… however I don’t remember how things are managed in a “multi-request”
scenario where beans get detached.

ModificationProxy, with all its complexity, properly manages both the single
request case and the multi-request case which is typical of our Wicket UI
(many ajax requests going on, besides form validation and the like).

This also brings some other little flaws like JAIInfo.getJAI()

returning the default JAI instance and setJAI being of no value,
making me think JAI should not be part of the configuration model but
obtained somehow else using the configuration given by JAIInfo.

Right, this should be fixed.

All in all, interesting discussion, but we’re about to release a 2.3.x with a serious
bug, and the above approach is not applicable.
Can someone review the fix and tell me if it’s ok to backport it to the 2.3.x series?

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

7

Regarding the patch it looks ok to me. A few trivial things.

  • contains some non relevant changes to a test class
  • the test case diverges from typical test naming conventions

Fine with the backport since the changes are constrained to ModificationProxy itself. If it starts to leak out then I think we should start to consider alternatives.

Regarding ditching ModificationProxy. We were faced with this decision back when we implemented the new catalog and decided to ditch the “dao” pattern. As andrea states a pure cloning approach has its issues which is partly why we decided to go with ModificationProxy which at the time was relatively simple. I totally agree though that it has grown quite complex (story of my life) and i myself wouldn’t mind seeing it go. But at this point not sure that is very realistic.

Perhaps if we decide in the future to once again re-implement the web ui we can revisit this once again and come up with something better. I say that since the UI is the biggest “user” of in place modification of catalog and config objects. There is the rest api as well but that interaction is quite a bit simpler.

$0.02

···

On Tue, Feb 12, 2013 at 12:42 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:

On Mon, Feb 11, 2013 at 8:08 PM, Gabriel Roldan <groldan@anonymised.com> wrote:

This imho would lead to an “easier place where to live” for
developers, with a clean separation from configuration and business
objects.
On the Catalog being referenced by *StoreInfo and maybe other Info
objects, it is solely in order to provide access to the catalog’s
ResourcePool inside DataStoreInfo.getDataStore() for example. Now,
whenever you have a handle on a DataStoreInfo (data object) and need
access to the DataStore (business object) you most probably have other
means to access the Catalog/ResourcePool and query it directly with
your DataStoreInfo or perhaps better with just its id.

At the moment the ResourcePool is “hidden”, there is no
direct usage in the services code, and *Info objects are passed around
in many places that have no direct access to the Catalog (as in, they lack
direct access to the Spring context, though that might be dodged using
GeoServerExtensions).

On the ModificationProxy proposal, my (rather limited) understanding
is it provides for the list of changed properties for catalog events.
Now, IMHO, the added complexity (ModificationProxy is a rather complex
beast itself) is not worth it compared to a design where *Info objects
are just POJOS that can be cloned/serialized with no extra care, where
the (default, in-memory) Catalog return clones of the queried objects
instead of proxies, and where when the changed properties need to be
figured out (like in for throwing catalog events) the two POJO
instances (the current and the one being modified) are run through
some sort of diff function that returns the list of changed
properties.

The approach as described is quite problematic. Here is an example:

  • Info object A is cloned by thread1, making B. Properties p1 and p2 get modified
  • Info object A is cloned in thread2, making C. Property p3 is modified
  • Thread1 saves, a comparison against the in-catalog copy A is made and p1 and p2
    get modified in A too
  • Thread2 saves, a comparisong against the in-catalog copy A is made and p3 is
    saved, p1 and p2 are reverted to their previous state

The above mishbehavior cannot happen using ModificationProxy, as the original
state of the bean is carried around.
While the solution is not the same, the behavior is like Hibernate one, where the
original state of the bean is kept in dehidrated form in the Session (that is, in a thread
local)… however I don’t remember how things are managed in a “multi-request”
scenario where beans get detached.

ModificationProxy, with all its complexity, properly manages both the single
request case and the multi-request case which is typical of our Wicket UI
(many ajax requests going on, besides form validation and the like).

This also brings some other little flaws like JAIInfo.getJAI()

returning the default JAI instance and setJAI being of no value,
making me think JAI should not be part of the configuration model but
obtained somehow else using the configuration given by JAIInfo.

Right, this should be fixed.

All in all, interesting discussion, but we’re about to release a 2.3.x with a serious
bug, and the above approach is not applicable.
Can someone review the fix and tell me if it’s ok to backport it to the 2.3.x series?

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.