memory leaks and thread-safety violations when deserializing types with object ids

[hearnden]

Problem:
The lifecycle of SimpleObjectIdResolver is obviously meant to be scoped by a call to Object{Mapper,Reader}.readValue(), since it is not threadsafe and accumulates ids forever. However, it is getting cached along with long-lived deserializers, and re-used across threads and across multiple calls to Object{Mapper,Reader}.readValue(). This violates thread safety and is a memory leak.

Jackson version: 2.4+ (ObjectIdResolver appears not to exist in <2.4).

How to reproduce:

• Make a class Foo that uses object ids with @JsonIdentityInfo
• Write code like

String json = mapper.writeValueAsString(...);
mapper.readValue(json, Foo.class);
mapper.readValue(json, Foo.class);

• Put breakpoint in SimpleObjectIdResolver's constructor

Expected results:
SimpleObjectIdResolver is constructed twice (once per call to readValue()).

Observed results:
SimpleObjectIdResolver is constructed only once.

[cowtowncoder]

Ouch. This sounds nasty. I did not write SimpleObjectIdResolver so I need to go and re-review the code; you are right that its life-cycle should align with individual calls and definitely not span across calls.

[cowtowncoder]

I really hope problem is not in SimpleObjectIdResolver, since that would be extremely unfortunate, it being in jackson-annotations package, for which no changes are planned in patch releases.... but it seems more likely that this is indeed in databind.

[cowtowncoder]

Looks almost as if some refactoring produced the problem, but short-circuiting things... only deprecated methods seem to be constructing SimpleObjectIdResolvers; code is wrong it looks like.

@pgelinas: any idea of possible fix here? Code is really, really wrong: it looks like ObjectIdResolver was set for BeanDeserializers, and that makes no sense -- they are long-lived, cached, and their life-cycle is different from ones that SimpleObjectIdResolver should have.
Or, alternatively, Map of object ids should not reside in SimpleObjectIdResolver but within context.

[pgelinas]

I'll take a look tonight, I seem to have overlooked something indeed. Definetly SimpleObjectIdResolver was not meant to be long-lived.

[pgelinas]

Yup, this isn't going nice to fix: the problem lies within SimpleObjectIdReader: the method newForDeserialization should construct a new instance of the class, which it doesn't. When I wrote this, I based myself on PropertyBasedObjectIdGenerator, which has no state and doesn't construct a new instance per serialization context.

Since the class is in the annotation package, the real fix will have to wait until 4.2 as I understand. So, what could be done instead is add a special case in DefaultDeserializationContext#findObjectId when calling newForDeserialization (around line 92 in my current sources). I'll have a PR coming soon.

[cowtowncoder]

@pgelinas Ah ok. Yes, I figured it was due to refactorings, a side-effect and definitely not intentional. I am ok with a patch, since you are correct in that no fixes for annotations package are to be made (except for javadocs and alike) for patch releases.
Thank you for coming up with a fix so quickly.

Unfortunately I just released 2.4.1 before seeing this (... that's how things often go), so patch will need to go to 2.4.2. But it's better to have a fix, test it, and be sure it works, rather than rush.

Alternatively I could also go with 2.4.1.1 only for jackson-databind -- this actually might make sense here, so anyone who urgently needs the fix can get it, with overrides; and others get it with full 2.4.2 release.

[cowtowncoder]

As per patch #491, work-around is included in 2.4.1.1 "micro-patch" (that is, not a full release, only jackson-databind released) and will be in 2.4.2 later on. Full fix then for 2.5.0.

[cowtowncoder]

@pgelinas Now that master is moved to 2.5.0, can this be fully fixed?

[ikolomiets]

Hi guys,

I guess what I see in this pic is your memory leak. I'm using Jackson 2.4.2 with JaxbAnnotationModule and running my app for a few days shows considerable increase in heap memory size.

When are you planning to release 2.5.0? Meanwhile, I'm going to try Jackson 2.3.4 to see if it doesn't have this issue.

[pgelinas]

Yes, I think this can be fixed with a new version of Jackson-annotations, I'll take a look at this shortly.
@ikolomiets The fix should be in 2.4.2. If you can test with a 2.3.x Jackson version and validate if you still have the leak, it would be appreciated.