Improve Jackson ObjectMapper usage

[hzalaz]

Ideally we should have a single ObjectMapper per Verifier/Creator instance since they are meant to be shared and there is no need to recreate them per request. However we are creating some additional mappers in some places that can be avoided and the alternatives are:

1. store in a static var a Mapper (not something I'd like)
2. make sure we have a single mapper per Verifier/Creator and pass it along to anyone that needs it
3. same as the previous one but instead of passing the mapper we pass the Reader/Writer

So far the preferred option is #3 but we can start with #2 and then improve later with Writer/Reader

Related to #140

[cacsar]

Not sure if this should be a new enhancement request or not, but the ability to pass in a custom ObjectMapper would be quite useful. It's possible we're just using an old version of Jackson (from Dropwizard), but classes with Optional get rejected because https://github.com/FasterXML/jackson-modules-java8 isn't registered. Whereas if we could inject a custom ObjectMapper we could use the same one we use elsewhere in our code base.

[lbalmaceda]

Hi @cacsar,
Adding a custom ObjectMapper means tying the public API to a specific library and we won't do that. If you have complex classes or just need to get that Optional type, you may parse them manually when #152 get's merged.
Cheers

[cacsar]

Ok, in that case I'd recommend registering the Jackson JDK8 module (provided you've dropped Java 7 support in this library) so that Java8 types (like Optional) are supported out of the box.

[thfro]

How about replacing Jackson with the newly releaed JSON-B 1.0, or its reference implementation respectively? It can be expected that many projects are planning to migrate to JSON-B in the near future, just because there now is a standard API for json binding. And it is tedious to have two binding frameworks in the classpath, only because the JWT library used has a dependency on a proprietary JSON binding solution.

[dmak]

How about replacing Jackson with the newly releaed JSON-B 1.0

...or wait until Jackson adopts JSON-B, see issue#19.

[skjolber]

So this almost solves itself when #194 is merged? The JWTParser constructor needs a tweak though.

[cbxp]

We are experiencing production JVM crashes on Java 10 and 11 inside of Jackson's introspector.
We generate lots of JWT tokens, and generation of each one creates a new ObjectMapper that starts the introspection of Java classes and methods over and over again and eventually crashes the JVM:
https://bugs.openjdk.java.net/browse/JDK-8210044

Maybe it's at least a good idea to tune ObjectMapper parameters and disable some introspection that is not used by JWT generation
https://github.com/FasterXML/jackson-databind/wiki/Mapper-Features

It seems that there are too many features enabled by default.

[skjolber]

See also #255 for reusing the ObjectMapper

[skjolber]

@cbxp @lbalmaceda so basically this would mean that token verification suffers from the same problem?

[angryziber]

Probably, but we are not running verification at this rate on Java 10 yet :-)

[skjolber]

Okey (I think perhaps we might be). So this bug, is it just a matter of time before the JVM crashes or does it have to be a frequent operation to fail?

[angryziber]

That's hard to tell - so far we could reproduce the crash only on production, so testing possibilities are limited. But seems that initializing Jackson only once avoids the problem and makes JWT generation faster. Jackson runs lots of complex code during initialization, so I would turn off any searches that are not needed as well.

[angryziber]

For some reason, there are quite a lot of JVM crashes reported in Java bug database related to Jackson if you Google for it

[skjolber]

@angryziber @cbxp we raised a ticket and the engineering team was notified, a fix is a few weeks out.

[skjolber]

@lbalmaceda any hope for resolving this in 2018? We're getting more and more JDK 11 applications.

[skjolber]

#255 has been merged, so that takes care of the JWT validation aspect.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

[angelo147]

Is still any interest about this? I have made an implementation of reusing 2 pools of ObjectMapper instances, one for JWTCreator and one for JWTParser. Seems to work fine for my needs at least.