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

HHH-14694 Don't clear BytecodeProvider cache when SessionFactory is built or closed #9831

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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 @@ -19,8 +19,6 @@
import org.hibernate.boot.spi.MetadataImplementor;
import org.hibernate.boot.spi.SessionFactoryBuilderImplementor;
import org.hibernate.boot.spi.SessionFactoryOptions;
import org.hibernate.bytecode.internal.SessionFactoryObserverForBytecodeEnhancer;
import org.hibernate.bytecode.spi.BytecodeProvider;
import org.hibernate.cache.spi.TimestampsCacheFactory;
import org.hibernate.context.spi.CurrentTenantIdentifierResolver;
import org.hibernate.internal.SessionFactoryImpl;
Expand Down Expand Up @@ -63,10 +61,11 @@ public SessionFactoryBuilderImpl(MetadataImplementor metadata, SessionFactoryOpt
}
}

final BytecodeProvider bytecodeProvider =
metadata.getMetadataBuildingOptions().getServiceRegistry()
.getService( BytecodeProvider.class );
addSessionFactoryObservers( new SessionFactoryObserverForBytecodeEnhancer( bytecodeProvider ) );
// Don't clear the state anymore, since the cache is not static anymore since HHH-16058 was fixed
// final BytecodeProvider bytecodeProvider =
// metadata.getMetadataBuildingOptions().getServiceRegistry()
// .getService( BytecodeProvider.class );
// addSessionFactoryObservers( new SessionFactoryObserverForBytecodeEnhancer( bytecodeProvider ) );
Comment on lines +64 to +68
Copy link
Member

Choose a reason for hiding this comment

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

Several things:

  1. Maybe remove the code instead of commenting it out?
  2. We use this observer in Quarkus: https://github.com/quarkusio/quarkus/blob/47ecd0a245b26e85a2b01536f182f3292e95b2e8/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootEntityManagerFactoryBuilder.java#L194-L198 . I think we should remove it there too, but let me know if you disagree.
  3. Maybe you should remove or at least deprecate this observer, if it's no longer useful?

addSessionFactoryObservers( new SessionFactoryObserverForNamedQueryValidation( metadata ) );
addSessionFactoryObservers( new SessionFactoryObserverForSchemaExport( metadata ) );
addSessionFactoryObservers( new SessionFactoryObserverForRegistration() );
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* SPDX-License-Identifier: LGPL-2.1-or-later
* Copyright Red Hat Inc. and Hibernate Authors
*/
package org.hibernate.bytecode.enhance.internal.bytebuddy;

import net.bytebuddy.pool.TypePool;

import java.util.AbstractMap;
import java.util.Map;

/**
* A simple cache provider that allows overriding the resolution for the class that is currently being enhanced.
*/
final class EnhancerCacheProvider extends TypePool.CacheProvider.Simple {

private final ThreadLocal<Map.Entry<String, TypePool.Resolution>> enhancedTypeResolution = new ThreadLocal<>();

@Override
public TypePool.Resolution find(final String name) {
final Map.Entry<String, TypePool.Resolution> entry = enhancedTypeResolution.get();
if ( entry != null && entry.getKey().equals( name ) ) {
return entry.getValue();
}
return super.find( name );
}

@Override
public void clear() {
super.clear();
enhancedTypeResolution.remove();
}

public void registerEnhancedType(final String className, TypePool.Resolution resolution) {
enhancedTypeResolution.set( new AbstractMap.SimpleEntry<>( className, resolution ) );
}

public void deregisterEnhancedType(final String name) {
enhancedTypeResolution.remove();
}

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'name' is never used.
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* SPDX-License-Identifier: LGPL-2.1-or-later
* Copyright Red Hat Inc. and Hibernate Authors
*/
package org.hibernate.bytecode.enhance.internal.bytebuddy;

import net.bytebuddy.dynamic.ClassFileLocator;

import java.io.IOException;
import java.util.AbstractMap;
import java.util.Map;

/**
* A delegating ClassFileLocator that allows overriding the resolution for the class that is currently being enhanced.
*/
final class EnhancerClassFileLocator implements ClassFileLocator {

private final ThreadLocal<Map.Entry<String, ClassFileLocator.Resolution>> enhancedTypeResolution = new ThreadLocal<>();
private final ClassFileLocator delegate;

public EnhancerClassFileLocator(ClassFileLocator delegate) {
this.delegate = delegate;
}

@Override
public Resolution locate(final String name) throws IOException {
final Map.Entry<String, ClassFileLocator.Resolution> entry = enhancedTypeResolution.get();
if ( entry != null && entry.getKey().equals( name ) ) {
return entry.getValue();
}
return delegate.locate( name );
}

@Override
public void close() throws IOException {
try {
delegate.close();
}
finally {
enhancedTypeResolution.remove();
}
}

public void registerEnhancedType(final String className, ClassFileLocator.Resolution resolution) {
enhancedTypeResolution.set( new AbstractMap.SimpleEntry<>( className, resolution ) );
}

public void deregisterEnhancedType(final String name) {
enhancedTypeResolution.remove();
}

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'name' is never used.

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
package org.hibernate.bytecode.enhance.internal.bytebuddy;

import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;

import net.bytebuddy.dynamic.ClassFileLocator;
import net.bytebuddy.pool.TypePool;
Expand All @@ -16,11 +15,10 @@
*/
public class ModelTypePool extends TypePool.Default implements EnhancerClassLocator {

private final ConcurrentHashMap<String, Resolution> resolutions = new ConcurrentHashMap<>();
private final OverridingClassFileLocator locator;
private final SafeCacheProvider poolCache;
private final EnhancerClassFileLocator locator;
private final EnhancerCacheProvider poolCache;

private ModelTypePool(SafeCacheProvider cacheProvider, OverridingClassFileLocator classFileLocator, CoreTypePool parent) {
private ModelTypePool(EnhancerCacheProvider cacheProvider, EnhancerClassFileLocator classFileLocator, CoreTypePool parent) {
super( cacheProvider, classFileLocator, ReaderMode.FAST, parent );
this.poolCache = cacheProvider;
this.locator = classFileLocator;
Expand Down Expand Up @@ -62,7 +60,7 @@ public static EnhancerClassLocator buildModelTypePool(ClassFileLocator classFile
* @return
*/
public static EnhancerClassLocator buildModelTypePool(ClassFileLocator classFileLocator, CoreTypePool coreTypePool) {
return buildModelTypePool( classFileLocator, coreTypePool, new SafeCacheProvider() );
return buildModelTypePool( classFileLocator, coreTypePool, new EnhancerCacheProvider() );
}

/**
Expand All @@ -72,39 +70,23 @@ public static EnhancerClassLocator buildModelTypePool(ClassFileLocator classFile
* @param cacheProvider
* @return
*/
public static EnhancerClassLocator buildModelTypePool(ClassFileLocator classFileLocator, CoreTypePool coreTypePool, SafeCacheProvider cacheProvider) {
static EnhancerClassLocator buildModelTypePool(ClassFileLocator classFileLocator, CoreTypePool coreTypePool, EnhancerCacheProvider cacheProvider) {
Objects.requireNonNull( classFileLocator );
Objects.requireNonNull( coreTypePool );
Objects.requireNonNull( cacheProvider );
return new ModelTypePool( cacheProvider, new OverridingClassFileLocator( classFileLocator ), coreTypePool );
}

@Override
protected Resolution doDescribe(final String name) {
final Resolution resolution = resolutions.get( name );
if ( resolution != null ) {
return resolution;
}
else {
return resolutions.computeIfAbsent( name, super::doDescribe );
}
return new ModelTypePool( cacheProvider, new EnhancerClassFileLocator( classFileLocator ), coreTypePool );
}

@Override
public void registerClassNameAndBytes(final String className, final byte[] bytes) {
//Very important: ensure the registered override is actually effective in case this class
//was already resolved in the recent past; this could have happened for example as a side effect
//of symbol resolution during enhancement of a different class, or very simply when attempting
//to re-enhanced the same class - which happens frequently in WildFly because of the class transformers
//being triggered concurrently by multiple parallel deployments.
resolutions.remove( className );
poolCache.remove( className );
locator.put( className, new ClassFileLocator.Resolution.Explicit( Objects.requireNonNull( bytes ) ) );
locator.registerEnhancedType( className, new ClassFileLocator.Resolution.Explicit( Objects.requireNonNull( bytes ) ) );
poolCache.registerEnhancedType( className, doDescribe( className ) );
}

@Override
public void deregisterClassNameAndBytes(final String className) {
locator.remove( className );
poolCache.deregisterEnhancedType( className );
locator.deregisterEnhancedType( className );
Comment on lines 87 to +89
Copy link
Member

Choose a reason for hiding this comment

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

I think you should remove the className parameter here, because it's misleading: it's not used in implementations, which will just remove the currently registered class. Yes in practice it should be the one whose name is being passed, but still, it's misleading.

}

@Override
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,6 @@ public byte[] transform(
catch (final Exception e) {
throw new TransformerException( "Error performing enhancement of " + className, e );
}
finally {
bytecodeProvider.resetCaches();
}
}

@Override
Expand Down
Loading