feat: Use singleton JsonFactory instance #31

[georgestoupis]

Re: issue #31

[CLAassistant]

All committers have signed the CLA.

[breedx-nr]

Thanks @gstoupis for this contribution! This is a nice start.

I left a comment about the race condition in the singleton, but I think it's an easy change.

My instinct would have been to just add a field of JsonFactory to the NewRelicJsonLayout, but this approach (pure singleton) should be fine too.

Thanks!

[breedx-nr]

So it might be a bit pedantic and may not seriously impact real-world applications, but technically there is a race condition here.

Imagine 10 threads, all calling getInstance() at the same time. It is possible for all 10 to pass the null check, then they all create instances and assign them and return them. For some period of time, there will be more than 1 instance of JsonFactory around.

In this case, especially since JsonFactory has no dependencies and is so easy to construct, I think it is easily remedied by changing the static field declaration to:

    private final static JsonFactory jsonFactory = new JsonFactory();

and then you can remove the if() in the getter. Should be safer and simpler that way.

[georgestoupis]

Good catch! I'll change it accordingly.

[georgestoupis]

Done in cb71c26

[georgestoupis]

Thanks @gstoupis for this contribution! This is a nice start.

I left a comment about the race condition in the singleton, but I think it's an easy change.

My instinct would have been to just add a field of JsonFactory to the NewRelicJsonLayout, but this approach (pure singleton) should be fine too.

Thanks!

Thanks for the review @breedx-nr! You're right, having it inside the NewRelicJsonLayout would be simpler. However, I took a look around other packages as well, and saw that they had the same logic (a new JsonFactory() in similar places each time).

So I thought that creating a new class and having it provide the singleton JsonFactory could be utilized by these other packages well.

Of course, if you think it'd be better, I can revert the changes and just add a field of JsonFactory to the NewRelicJsonLayout, as you propose. Otherwise, I'll fix the race condition you mentioned above, and leave the rest as is.

What do you think?

[breedx-nr]

I think that your approach is solid and the ability to reuse in other packages is forward-looking, so let's go with that.
Thanks! 🥂

[breedx-nr]

Thanks again!