Skip to content
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.

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

Log "excessive" creation of factories and proxies #690

Merged
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
import org.slf4j.LoggerFactory;

import javax.inject.Inject;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.PrintWriter;
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
Expand All @@ -24,13 +27,15 @@
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.lang.reflect.Type;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.SortedSet;
import java.util.WeakHashMap;
import java.util.function.Function;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -98,6 +103,12 @@
public class ConfigProxyFactory {
private static final Logger LOG = LoggerFactory.getLogger(ConfigProxyFactory.class);

// Users sometimes leak both factories and proxies, leading to memory problems that get blamed on us ;-)

Choose a reason for hiding this comment

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

IMHO it would be nice to rewrite the message in lieu of the "true" Netflix culture instead of continuing "blame" culture :)

// We'll use these maps to keep track of how many instances of each are created and make log noise to help them

Choose a reason for hiding this comment

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

extra nit: code style: different javadoc style from the rest of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really a javadoc, more an internal comment for implementors :-)

// track down the actual culprits. WeakHashMaps to avoid holding onto objects ourselves.
private static final Map<Config, Integer> FACTORIES_COUNT = Collections.synchronizedMap(new WeakHashMap<>());
private static final Map<Class<?>, Integer> PROXIES_COUNT = Collections.synchronizedMap(new WeakHashMap<>());

/**
* The decoder is used for the purpose of decoding any @DefaultValue annotation
*/
Expand All @@ -121,6 +132,8 @@ public ConfigProxyFactory(Config config, Decoder decoder, PropertyFactory factor
this.decoder = decoder;
this.config = config;
this.propertyRepository = factory;

warnWhenTooMany(FACTORIES_COUNT, config, 5, String.format("ProxyFactory(Config:%s)", config.hashCode()));
}

/**
Expand Down Expand Up @@ -189,6 +202,8 @@ interface PropertyValueGetter<T> {

@SuppressWarnings({"unchecked", "rawtypes"})
<T> T newProxy(final Class<T> type, final String initialPrefix, boolean immutable) {
warnWhenTooMany(PROXIES_COUNT, type, 50, String.format("Proxy(%s)", type));

Choose a reason for hiding this comment

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

would it make sense to make magic constants configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trick here is that some of this code could be running before config is fully initialized. Let me think it through.

Copy link
Contributor

Choose a reason for hiding this comment

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

50 seems a little high for this in general, is this to accommodate the possibility of someone having the same proxy class but with different prefixes? If so, would it make sense to combine that into the key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's the thought.


Configuration annot = type.getAnnotation(Configuration.class);

final String prefix = derivePrefix(annot, initialPrefix);
Expand Down Expand Up @@ -460,6 +475,29 @@ private static void maybeWrapThenRethrow(Throwable t) {
throw new RuntimeException(t);
}

private static <T> void warnWhenTooMany(Map<T, Integer> counters, T countKey, int limit, String objectDescription) {
int currentCount = counters.merge(countKey, 1, Integer::sum);

// Emit warning if we're over the limit BUT only when the current count is a multiple of the limit :-)
// This is to avoid being *too* noisy
if (LOG.isWarnEnabled() &&
currentCount >= limit &&
(currentCount % limit == 0 )) {

String stackTraceForDebuggint;

Choose a reason for hiding this comment

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

nit: typo in stackTraceForDebuggint? (debugging)

try (ByteArrayOutputStream os = new ByteArrayOutputStream(4000);
PrintWriter pw = new PrintWriter(os)) {
new RuntimeException().printStackTrace(pw);
pw.flush();
stackTraceForDebuggint = os.toString(StandardCharsets.UTF_8.name());
} catch (IOException e) {
stackTraceForDebuggint = "<MISSING>";
}

LOG.warn("Too many {} are being created. Please review the calling code to prevent memory leaks. Stack trace for debugging follows {}",
objectDescription, stackTraceForDebuggint);
}
}

/** InvocationHandler for config proxies. */
private static class ConfigProxyInvocationHandler<P> implements InvocationHandler {
Expand Down