field-reference: cap RUBY_CACHE to 10k entries (#13642) (#13655)

* field-reference: cap RUBY_CACHE to 10k entries

Reduces the scope of a memory leak that can be caused by using UUIDs or other
high-cardinality field names by preventing the ruby string _keys_ from being
held by the cache indefinitely.

Note: this may not solve the problem entirely, but certainly limits its impact.
      Because ConvertedMap requires individual field names to be interned into
      the global String intern pool, their eligibility for GC is JVM-specific
      and high-cardinality field names should still be avoided.

* noop: field-reference test refactor to consolodate reflection

(cherry picked from commit ca501acdcf)
This commit is contained in:
Ry Biesemeyer 2022-01-20 09:18:43 -08:00 committed by GitHub
parent 3c0b948a85
commit c8e2bef09b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 39 additions and 25 deletions

View file

@ -118,7 +118,13 @@ public final class FieldReference {
if (result != null) {
return result;
}
return RUBY_CACHE.computeIfAbsent(reference.newFrozen(), ref -> from(ref.asJavaString()));
final FieldReference parsed = from(reference.asJavaString());
// exact size in a race condition is not important
if (RUBY_CACHE.size() < 10_000) {
RUBY_CACHE.put(reference.newFrozen(), parsed);
}
return parsed;
}
public static FieldReference from(final String reference) {

View file

@ -23,6 +23,8 @@ package org.logstash;
import java.lang.reflect.Field;
import java.util.Map;
import org.hamcrest.CoreMatchers;
import org.jruby.RubyString;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@ -33,25 +35,12 @@ import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
public final class FieldReferenceTest {
@SuppressWarnings("unchecked")
@Before
public void clearParsingCache() throws Exception {
final Field cacheField = FieldReference.class.getDeclaredField("CACHE");
cacheField.setAccessible(true);
final Map<CharSequence, FieldReference> cache =
(Map<CharSequence, FieldReference>) cacheField.get(null);
cache.clear();
}
@SuppressWarnings("unchecked")
@Before
public void clearDedupCache() throws Exception {
final Field cacheField = FieldReference.class.getDeclaredField("DEDUP");
cacheField.setAccessible(true);
final Map<CharSequence, FieldReference> cache =
(Map<CharSequence, FieldReference>) cacheField.get(null);
cache.clear();
@After
public void clearInternalCaches() {
getInternalCache("CACHE").clear();
getInternalCache("DEDUP").clear();
getInternalCache("RUBY_CACHE").clear();
}
@Test
@ -68,13 +57,9 @@ public final class FieldReferenceTest {
);
}
@SuppressWarnings("unchecked")
@Test
public void testCacheUpperBound() throws NoSuchFieldException, IllegalAccessException {
final Field cacheField = FieldReference.class.getDeclaredField("CACHE");
cacheField.setAccessible(true);
final Map<CharSequence, FieldReference> cache =
(Map<CharSequence, FieldReference>) cacheField.get(null);
public void testCacheUpperBound() {
final Map<String, FieldReference> cache = getInternalCache("CACHE");
final int initial = cache.size();
for (int i = 0; i < 10_001 - initial; ++i) {
FieldReference.from(String.format("[array][%d]", i));
@ -82,6 +67,17 @@ public final class FieldReferenceTest {
assertThat(cache.size(), CoreMatchers.is(10_000));
}
@Test
public void testRubyCacheUpperBound() {
final Map<RubyString, FieldReference> cache = getInternalCache("RUBY_CACHE");
final int initial = cache.size();
for (int i = 0; i < 10_050 - initial; ++i) {
final RubyString rubyString = RubyUtil.RUBY.newString(String.format("[array][%d]", i));
FieldReference.from(rubyString);
}
assertThat(cache.size(), CoreMatchers.is(10_000));
}
@Test
public void testParseSingleBareField() throws Exception {
FieldReference f = FieldReference.from("foo");
@ -183,4 +179,16 @@ public final class FieldReferenceTest {
public void testParseLiteralSquareBrackets() throws Exception {
FieldReference.from("this[index]");
}
@SuppressWarnings("unchecked")
private <K,V> Map<K,V> getInternalCache(final String fieldName) {
final Field cacheField;
try {
cacheField = FieldReference.class.getDeclaredField(fieldName);
cacheField.setAccessible(true);
return (Map<K, V>) cacheField.get(null);
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new RuntimeException(e);
}
}
}