Skip to content

Cache MetadataReader by class name to avoid duplicate metadata #36977

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

Open
LordKay-sudo wants to merge 1 commit into
base: main
Choose a base branch
from
+266 −19
Open

Cache MetadataReader by class name to avoid duplicate metadata #36977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
205 changes: 186 additions & 19 deletions ...rc/main/java/org/springframework/core/type/classreading/CachingMetadataReaderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,27 @@
package org.springframework.core.type.classreading;

import java.io.IOException;
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;

import org.jspecify.annotations.Nullable;

import org.springframework.core.io.ClassPathResource;
import org.springframework.core.io.DefaultResourceLoader;
import org.springframework.core.io.Resource;
import org.springframework.core.io.ResourceLoader;
import org.springframework.util.ClassUtils;
import org.springframework.util.ResourceUtils;

/**
* Caching implementation of the {@link MetadataReaderFactory} interface,
* caching a {@link MetadataReader} instance per Spring {@link Resource} handle
* (i.e. per ".class" file).
* caching a {@link MetadataReader} instance per class name.
* <p>Metadata is also associated with the originating {@link Resource} handle
* in the shared {@link DefaultResourceLoader} resource cache when available.
*
* @author Juergen Hoeller
* @author Costin Leau
Expand All @@ -42,8 +49,15 @@ public class CachingMetadataReaderFactory extends AbstractMetadataReaderFactory
/** Default maximum number of entries for a local MetadataReader cache: 256. */
public static final int DEFAULT_CACHE_LIMIT = 256;

private static final String CLASSES_DIR = "/classes/";

private static final String TEST_CLASSES_DIR = "/test-classes/";

private final MetadataReaderFactory delegate;

/** MetadataReader cache keyed by class name. */
private @Nullable Map<String, MetadataReader> classNameMetadataReaderCache;

/** MetadataReader cache: either local or shared at the ResourceLoader level. */
private @Nullable Map<Resource, MetadataReader> metadataReaderCache;

Expand Down Expand Up @@ -81,6 +95,7 @@ public CachingMetadataReaderFactory(@Nullable ResourceLoader resourceLoader) {
this.delegate = delegate;
if (getResourceLoader() instanceof DefaultResourceLoader defaultResourceLoader) {
this.metadataReaderCache = defaultResourceLoader.getResourceCache(MetadataReader.class);
this.classNameMetadataReaderCache = new ConcurrentHashMap<>();
}
else {
setCacheLimit(DEFAULT_CACHE_LIMIT);
Expand All @@ -97,12 +112,17 @@ public CachingMetadataReaderFactory(@Nullable ResourceLoader resourceLoader) {
public void setCacheLimit(int cacheLimit) {
if (cacheLimit <= 0) {
this.metadataReaderCache = null;
this.classNameMetadataReaderCache = null;
}
else if (this.metadataReaderCache instanceof LocalResourceCache localResourceCache) {
localResourceCache.setCacheLimit(cacheLimit);
if (this.classNameMetadataReaderCache instanceof LocalClassNameCache localClassNameCache) {
localClassNameCache.setCacheLimit(cacheLimit);
}
}
else {
this.metadataReaderCache = new LocalResourceCache(cacheLimit);
this.classNameMetadataReaderCache = new LocalClassNameCache(cacheLimit);
}
}

Expand All @@ -120,34 +140,49 @@ public int getCacheLimit() {

@Override
public MetadataReader getMetadataReader(Resource resource) throws IOException {
if (this.metadataReaderCache instanceof ConcurrentMap) {
// No synchronization necessary...
MetadataReader metadataReader = this.metadataReaderCache.get(resource);
if (metadataReader == null) {
metadataReader = this.delegate.getMetadataReader(resource);
this.metadataReaderCache.put(resource, metadataReader);
@Nullable String className = resolveClassName(resource);
if (className != null) {
MetadataReader metadataReader = getFromClassNameCache(className);
if (metadataReader != null) {
cacheMetadataReader(resource, metadataReader);
return metadataReader;
}
return metadataReader;
}
else if (this.metadataReaderCache != null) {
synchronized (this.metadataReaderCache) {
MetadataReader metadataReader = this.metadataReaderCache.get(resource);
if (metadataReader == null) {
metadataReader = this.delegate.getMetadataReader(resource);
this.metadataReaderCache.put(resource, metadataReader);
}
return metadataReader;

MetadataReader metadataReader = getFromResourceCache(resource);
if (metadataReader != null) {
if (className != null) {
cacheMetadataReaderByClassName(className, metadataReader);
}
return metadataReader;
}
else {
return this.delegate.getMetadataReader(resource);

metadataReader = this.delegate.getMetadataReader(resource);
if (className == null) {
className = metadataReader.getClassMetadata().getClassName();
}
MetadataReader existing = getFromClassNameCache(className);
if (existing != null) {
cacheMetadataReader(resource, existing);
return existing;
}
cacheMetadataReader(resource, metadataReader);
cacheMetadataReaderByClassName(className, metadataReader);
return metadataReader;
}

/**
* Clear the local MetadataReader cache, if any, removing all cached class metadata.
*/
public void clearCache() {
if (this.classNameMetadataReaderCache instanceof LocalClassNameCache) {
synchronized (this.classNameMetadataReaderCache) {
this.classNameMetadataReaderCache.clear();
}
}
else if (this.classNameMetadataReaderCache != null) {
this.classNameMetadataReaderCache.clear();
}
if (this.metadataReaderCache instanceof LocalResourceCache) {
synchronized (this.metadataReaderCache) {
this.metadataReaderCache.clear();
Expand All @@ -159,6 +194,114 @@ else if (this.metadataReaderCache != null) {
}
}

private @Nullable MetadataReader getFromClassNameCache(String className) {
if (this.classNameMetadataReaderCache instanceof ConcurrentMap<String, MetadataReader> concurrentMap) {
return concurrentMap.get(className);
}
else if (this.classNameMetadataReaderCache != null) {
synchronized (this.classNameMetadataReaderCache) {
return this.classNameMetadataReaderCache.get(className);
}
}
return null;
}

private @Nullable MetadataReader getFromResourceCache(Resource resource) {
if (this.metadataReaderCache instanceof ConcurrentMap<Resource, MetadataReader> concurrentMap) {
return concurrentMap.get(resource);
}
else if (this.metadataReaderCache != null) {
synchronized (this.metadataReaderCache) {
return this.metadataReaderCache.get(resource);
}
}
return null;
}

private void cacheMetadataReader(Resource resource, MetadataReader metadataReader) {
if (this.metadataReaderCache instanceof ConcurrentMap<Resource, MetadataReader> concurrentMap) {
concurrentMap.putIfAbsent(resource, metadataReader);
}
else if (this.metadataReaderCache != null) {
synchronized (this.metadataReaderCache) {
this.metadataReaderCache.putIfAbsent(resource, metadataReader);
}
}
}

private void cacheMetadataReaderByClassName(MetadataReader metadataReader) {
cacheMetadataReaderByClassName(metadataReader.getClassMetadata().getClassName(), metadataReader);
}

private void cacheMetadataReaderByClassName(String className, MetadataReader metadataReader) {
if (this.classNameMetadataReaderCache instanceof ConcurrentMap<String, MetadataReader> concurrentMap) {
concurrentMap.putIfAbsent(className, metadataReader);
}
else if (this.classNameMetadataReaderCache != null) {
synchronized (this.classNameMetadataReaderCache) {
this.classNameMetadataReaderCache.putIfAbsent(className, metadataReader);
}
}
}

static @Nullable String resolveClassName(Resource resource) {
if (resource instanceof ClassPathResource classPathResource) {
return toClassName(classPathResource.getPath());
}
try {
String urlString = resource.getURL().toString();
int separatorIndex = urlString.indexOf(ResourceUtils.JAR_URL_SEPARATOR);
String path = (separatorIndex >= 0 ?
urlString.substring(separatorIndex + ResourceUtils.JAR_URL_SEPARATOR.length()) :
resource.getURL().getPath());
path = URLDecoder.decode(path, StandardCharsets.UTF_8);
if (path.startsWith("/")) {
path = path.substring(1);
}
return toClassName(extractClassFileResourcePath(path));
}
catch (IOException ex) {
return null;
}
}

private static final String[] CLASS_FILE_ROOT_MARKERS = {
"/classes/java/test/", "/classes/java/main/",
"/classes/kotlin/test/", "/classes/kotlin/main/",
CLASSES_DIR, TEST_CLASSES_DIR
};

private static String extractClassFileResourcePath(String path) {
int classFileIndex = path.indexOf(ClassUtils.CLASS_FILE_SUFFIX);
if (classFileIndex < 0) {
return path;
}
String beforeSuffix = path.substring(0, classFileIndex);
int bestStart = -1;
for (String marker : CLASS_FILE_ROOT_MARKERS) {
int index = beforeSuffix.indexOf(marker);
if (index >= 0) {
int start = index + marker.length();
if (start > bestStart) {
bestStart = start;
}
}
}
if (bestStart >= 0) {
return path.substring(bestStart, classFileIndex + ClassUtils.CLASS_FILE_SUFFIX.length());
}
return path;
}

private static @Nullable String toClassName(String resourcePath) {
if (!resourcePath.endsWith(ClassUtils.CLASS_FILE_SUFFIX)) {
return null;
}
String pathWithoutSuffix = resourcePath.substring(0,
resourcePath.length() - ClassUtils.CLASS_FILE_SUFFIX.length());
return ClassUtils.convertResourcePathToClassName(pathWithoutSuffix);
}


@SuppressWarnings("serial")
private static class LocalResourceCache extends LinkedHashMap<Resource, MetadataReader> {
Expand All @@ -184,4 +327,28 @@ protected boolean removeEldestEntry(Map.Entry<Resource, MetadataReader> eldest)
}
}

@SuppressWarnings("serial")
private static class LocalClassNameCache extends LinkedHashMap<String, MetadataReader> {

private volatile int cacheLimit;

public LocalClassNameCache(int cacheLimit) {
super(cacheLimit, 0.75f, true);
this.cacheLimit = cacheLimit;
}

public void setCacheLimit(int cacheLimit) {
this.cacheLimit = cacheLimit;
}

public int getCacheLimit() {
return this.cacheLimit;
}

@Override
protected boolean removeEldestEntry(Map.Entry<String, MetadataReader> eldest) {
return size() > this.cacheLimit;
}
}

}
80 changes: 80 additions & 0 deletions ...st/java/org/springframework/core/type/classreading/CachingMetadataReaderFactoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,19 @@

package org.springframework.core.type.classreading;

import java.net.URL;
import java.util.Map;

import org.junit.jupiter.api.Test;

import org.springframework.core.io.ClassPathResource;
import org.springframework.core.io.DefaultResourceLoader;
import org.springframework.core.io.Resource;
import org.springframework.core.io.UrlResource;
import org.springframework.core.type.ClassMetadata;
import org.springframework.util.ClassUtils;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
Expand All @@ -43,6 +52,77 @@ void shouldCacheClassNameCalls() throws Exception {
verify(delegate, times(1)).getMetadataReader(any(Resource.class));
}

/**
* Reproduces gh-36737: the same class loaded via ClassPathResource and UrlResource
* must not create duplicate MetadataReader instances.
*/
@Test
void shouldNotDuplicateMetadataForSameClassFromDifferentResources() throws Exception {
MetadataReaderFactory delegate = mock(MetadataReaderFactory.class);
when(delegate.getMetadataReader(any(Resource.class))).thenAnswer(invocation -> {
Resource resource = invocation.getArgument(0);
MetadataReader reader = mock(MetadataReader.class);
ClassMetadata classMetadata = mock(ClassMetadata.class);
when(classMetadata.getClassName()).thenReturn(TestClass.class.getName());
when(reader.getClassMetadata()).thenReturn(classMetadata);
when(reader.getResource()).thenReturn(resource);
return reader;
});

CachingMetadataReaderFactory readerFactory = new CachingMetadataReaderFactory(delegate);

String classFilePath = ClassUtils.convertClassNameToResourcePath(TestClass.class.getName()) +
ClassUtils.CLASS_FILE_SUFFIX;
Resource classpathResource = new ClassPathResource(classFilePath);
URL url = TestClass.class.getResource('/' + classFilePath);
assertThat(url).isNotNull();
Resource urlResource = new UrlResource(url);

readerFactory.getMetadataReader(classpathResource);
readerFactory.getMetadataReader(urlResource);

verify(delegate, times(1)).getMetadataReader(any(Resource.class));
}

@Test
void shouldResolveClassNameFromDifferentResourceRepresentations() throws Exception {
String classFilePath = ClassUtils.convertClassNameToResourcePath(TestClass.class.getName()) +
ClassUtils.CLASS_FILE_SUFFIX;
Resource classpathResource = new ClassPathResource(classFilePath);
URL url = TestClass.class.getResource('/' + classFilePath);
assertThat(url).isNotNull();
Resource urlResource = new UrlResource(url);

assertThat(CachingMetadataReaderFactory.resolveClassName(classpathResource))
.isEqualTo(TestClass.class.getName());
assertThat(CachingMetadataReaderFactory.resolveClassName(urlResource))
.isEqualTo(TestClass.class.getName());
}

@Test
void shouldReuseMetadataReaderForSameClassFromDifferentResources() throws Exception {
String classFilePath = ClassUtils.convertClassNameToResourcePath(TestClass.class.getName()) +
ClassUtils.CLASS_FILE_SUFFIX;
Resource classpathResource = new ClassPathResource(classFilePath);
URL url = TestClass.class.getResource('/' + classFilePath);
assertThat(url).isNotNull();
Resource urlResource = new UrlResource(url);

DefaultResourceLoader resourceLoader = new DefaultResourceLoader();
CachingMetadataReaderFactory readerFactory = new CachingMetadataReaderFactory(resourceLoader);
MetadataReader fromClasspath = readerFactory.getMetadataReader(classpathResource);
MetadataReader fromUrl = readerFactory.getMetadataReader(urlResource);

assertThat(fromUrl).isSameAs(fromClasspath);

Map<Resource, MetadataReader> sharedCache = resourceLoader.getResourceCache(MetadataReader.class);
long distinctReadersForSameClass = sharedCache.values().stream()
.filter(reader -> TestClass.class.getName().equals(reader.getClassMetadata().getClassName()))
.distinct()
.count();
assertThat(distinctReadersForSameClass).isOne();
}

public static class TestClass {
}

Expand Down