Skip to content

feat: Use singleton JsonFactory instance #31 #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
breedx-nr merged 4 commits into from
Oct 6, 2020
Merged

feat: Use singleton JsonFactory instance #31 #32

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
84ae6f8
feat: Use singleton JsonFactory instance #31
georgestoupis Oct 3, 2020
f8804a4
chore: remove unused import
georgestoupis Oct 3, 2020
3bbc5e9
test: add unit test for JsonFactoryProviderTest
georgestoupis Oct 3, 2020
cb71c26
fix: avoid race condition in JsonFactoryProvider
georgestoupis Oct 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions logback/src/main/java/com/newrelic/logging/logback/JsonFactoryProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.newrelic.logging.logback;

import com.fasterxml.jackson.core.JsonFactory;

public class JsonFactoryProvider {
private static JsonFactory jsonFactory;

static JsonFactory getInstance() {
if (jsonFactory == null) {
Copy link
Contributor

@breedx-nr breedx-nr Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@georgestoupis georgestoupis Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I'll change it accordingly.

Copy link
Contributor Author

@georgestoupis georgestoupis Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cb71c26

jsonFactory = new JsonFactory();
}
return jsonFactory;
}

private JsonFactoryProvider() {

}
}
3 changes: 1 addition & 2 deletions logback/src/main/java/com/newrelic/logging/logback/NewRelicJsonLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import ch.qos.logback.classic.spi.IThrowableProxy;
import ch.qos.logback.classic.spi.StackTraceElementProxy;
import ch.qos.logback.core.LayoutBase;
import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonGenerator;
import com.newrelic.logging.core.ElementName;
import com.newrelic.logging.core.ExceptionUtil;
Expand All @@ -27,7 +26,7 @@ public class NewRelicJsonLayout extends LayoutBase<ILoggingEvent> {
public String doLayout(ILoggingEvent event) {
StringWriter sw = new StringWriter();

try (JsonGenerator generator = new JsonFactory().createGenerator(sw)) {
try (JsonGenerator generator = JsonFactoryProvider.getInstance().createGenerator(sw)) {
writeToGenerator(event, generator);
} catch (Throwable ignored) {
return event.getFormattedMessage();
Expand Down
21 changes: 21 additions & 0 deletions logback/src/test/java/com/newrelic/logging/logback/JsonFactoryProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package com.newrelic.logging.logback;

import com.fasterxml.jackson.core.JsonFactory;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertSame;

class JsonFactoryProviderTest {

@Test
void shouldAlwaysGetSameInstance(){
JsonFactory instance1 = JsonFactoryProvider.getInstance();
JsonFactory instance2 = JsonFactoryProvider.getInstance();

assertNotNull(instance1);
assertNotNull(instance2);
assertSame(instance1, instance2);
}

}