Warning regarding thread-safety / reentrant usage of PrettyPrinter and Indenter instances

[mhcom...@gmail.com]

Hello,

Today I ran into a bizarre issue where my application indented its JSON improperly or not at all, sometimes always wrong, and sometimes only partially wrong, and sometimes changing what kind of wrong from REST call to REST call.

After researching the problem, I noticed that PrettyPrinter and Indenter are not thread-safe, but do not mention this in their API documentation, as far as I could read during my investigation (using Jackson 2.9.5).

I am not 100% sure I have fixed this properly in my code, because it's extremely complicated. It appears that ObjectMapper is thread-safe, meaning you can re-use a static ObjectMapper in multiple parts of the code, however, it would seem that calling mapper.setDefaultPrettyPrinter(...) would potentially mean that it's not-thread-safe anymore based on the issue I was seeing

I had to make a factory method of sorts to generate a pretty-printer:

    public static DefaultPrettyPrinter getPrinter() {

        DefaultPrettyPrinter.Indenter indenter =

            new DefaultIndenter("    ", DefaultIndenter.SYS_LF);

        DefaultPrettyPrinter printer = new DefaultPrettyPrinter();

        printer.indentObjectsWith(indenter);

        printer.indentArraysWith(indenter);

        return printer;

    }

Then I needed this in my JsonFactory:

    public static class CustomJsonFactory extends JsonFactory {

        private static final long serialVersionUID = -3521368899846283407L;

        @Override

        protected JsonGenerator _createGenerator(Writer out, IOContext ctxt) throws IOException {

            DefaultPrettyPrinter printer = getPrinter();

            return super._createGenerator(out, ctxt).setPrettyPrinter(printer);

        }

        @Override

        protected JsonGenerator _createUTF8Generator(OutputStream out, IOContext ctxt) throws IOException {

            DefaultPrettyPrinter printer = getPrinter();

            return super._createUTF8Generator(out, ctxt).setPrettyPrinter(printer);

        }

    }

However, I am not sure if this fix will help on the static copies of ObjectMapper I set up in the code, for doing stuff like internally converting YAML to Objects, Objects to JSON strings for log messages, etc.

If anybody has some more experience how this should work, I'd love recommendations, and I would be happy to help write some kind of documentation patch if I can understand how this should be done properly to avoid breakage.

Matthew.

[Tatu Saloranta]

On Wed, Apr 25, 2018 at 4:16 PM, <mhcom...@gmail.com> wrote:
> Hello,
>
> Today I ran into a bizarre issue where my application indented its JSON
> improperly or not at all, sometimes always wrong, and sometimes only
> partially wrong, and sometimes changing what kind of wrong from REST call to
> REST call.
>
> After researching the problem, I noticed that PrettyPrinter and Indenter are
> not thread-safe, but do not mention this in their API documentation, as far
> as I could read during my investigation (using Jackson 2.9.5).

Have another look at `PrettyPrinter` Javadoc, this is expalined there, in second
paragraph (wrt `Instantiatable`).

So: to make sure `PrettyPrinter` works in thread-safe manner, it needs
to either:

1. Be stateless (like `MinimalPrettyPrinter`), immutable. OR
2. Implement `com.fasterxml.jackson.core.util.Instantiatable`,
implement `createInstance()` method
that creates stateful instance that is not shared across threads

`DefaultPrettyPrinter` does the latter. If you want to sub-class it,
however, you MUST override `createInstance()`
method; otherwise it will probably not work the way you want (since by
default it can not create instance of sub-class).

So use of `DefaultPrettyPrinter` should be fine without any extra work.
Assuming that all code paths in Jackson do use `createInstance`. So
one thing that could be useful is to see how
your code is creating generators. With that, it should be possible to
create actual test case with custom `PrettyPrinter`
that verifies that a copy must have been made (keep a flag or
something, set on "blueprint" instance, cleared for
actual instances created).

-+ Tatu +-

[mhcom...@gmail.com]

This is indeed confusing, because I did just use DefaultPrettyPrinter with some settings changes, and not a subclass, but the thread behavior was clearly still wrong. I am assuming something must be bad about my JsonFactory code:

super._createGenerator(out, ctxt).setPrettyPrinter(printer);

I think maybe I need to switch that code to this instead?

return super._createGenerator(out, ctxt).setPrettyPrinter(printer.createInstance());

Matthew.

[Tatu Saloranta]

Ah. Any method starting with underscore is considered more or less
implementation-specific.
So you would need to sort of follow control flow from public methods.
Your suggestion sounds reasonable, assuming you know the type of
pretty printer in question
is DefaultPrettyPrinter.

-+ Tatu +-

[mhcom...@gmail.com]

On Wednesday, April 25, 2018 at 11:52:17 PM UTC-7, Tatu Saloranta wrote:

Ah. Any method starting with underscore is considered more or less
implementation-specific.
So you would need to sort of follow control flow from public methods.
Your suggestion sounds reasonable, assuming you know the type of
pretty printer in question
is DefaultPrettyPrinter.

-+ Tatu +-

Yeah, I didn't really want to have to do it this way, because it looked like a lot of secret internal code, but it was the only proven way to wedge some properly formatted JSON into the middle of Jersey 2, Grizzly, etc. You don't want to know how much code I was reading to get this to work initially... ;)

Matthew.

[Tatu Saloranta]

:)

Amen to that.

Approach taken by Jackson wrt `PrettyPrinter` is not the cleanest
either, and was retrofit to deal with
(in hindsight obvious) concurrency issues. So I don't blame you for
finding it non-intuitive.

But all's well that ends well -- yes, it is necessary to call
`createInstance` if using alternate entry points
when creating/using generators. Let me know if you have further issues
with this approach.

-+ Tatu +-