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-7910 Transaction timeout can cause non-threadsafe session access by reaper thread #476

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -623,4 +623,13 @@ public interface AvailableSettings {
public static final String USE_DIRECT_REFERENCE_CACHE_ENTRIES = "hibernate.cache.use_reference_entries";

public static final String USE_NATIONALIZED_CHARACTER_DATA = "hibernate.use_nationalized_character_data";

/**
* A transaction can be rolled back by another thread -- not the original
* application. Examples of this include a JTA transaction timeout handled
* by a background reaper thread. The ability to handle this situation
* requires checking the Thread ID every time Session is called. This can
* certainly have performance considerations. Default is enabled;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to post this comment so late (maybe consider for 4.3.x). If performance is impacted, perhaps we could defer to a platform callback that determines if the current thread is a background thread or not.

I'm not yet sure what I will on the AS7 container side, but I need a way to immediately tell if the current thread is a "Transaction Reaper Worker N" thread. No promises yet but if I can nail that down, I could also implement the above callback to help Hibernate know the same. Other JTA environments could possibly provide the same or use this current patch as the default if there is no better way.

*/
public static final String ENABLE_JTA_THREAD_HANDLING = "hibernate.enable_jta_thread_handling";
}
10 changes: 10 additions & 0 deletions hibernate-core/src/main/java/org/hibernate/cfg/Settings.java
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ public final class Settings {
private MultiTableBulkIdStrategy multiTableBulkIdStrategy;
private BatchFetchStyle batchFetchStyle;
private boolean directReferenceCacheEntriesEnabled;

private boolean jtaThreadHandling;


/**
Expand Down Expand Up @@ -508,4 +510,12 @@ public void setDirectReferenceCacheEntriesEnabled(boolean directReferenceCacheEn
void setDefaultNullPrecedence(NullPrecedence defaultNullPrecedence) {
this.defaultNullPrecedence = defaultNullPrecedence;
}

public boolean isJtaThreadHandling() {
return jtaThreadHandling;
}

public void setJtaThreadHandling(boolean jtaThreadHandling) {
this.jtaThreadHandling = jtaThreadHandling;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,16 @@ public Settings buildSettings(Properties props, ServiceRegistry serviceRegistry)
}
settings.setInitializeLazyStateOutsideTransactions( initializeLazyStateOutsideTransactionsEnabled );

boolean enableJtaThreadHandling = ConfigurationHelper.getBoolean(
AvailableSettings.ENABLE_JTA_THREAD_HANDLING,
properties,
true
);
if ( debugEnabled ) {
LOG.debugf( "JTA Thread Handling: %s", enabledDisabled(enableJtaThreadHandling) );
}
settings.setJtaThreadHandling( enableJtaThreadHandling );

return settings;

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ public SynchronizationCallbackCoordinator getSynchronizationCallbackCoordinator(
}

public void pulse() {
getSynchronizationCallbackCoordinator().pulse();
if ( transactionFactory().compatibleWithJtaSynchronization() ) {
// the configured transaction strategy says it supports callbacks via JTA synchronization, so attempt to
// register JTA synchronization if possible
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@

import javax.transaction.SystemException;

import org.jboss.logging.Logger;

import org.hibernate.TransactionException;
import org.hibernate.cfg.Settings;
import org.hibernate.engine.transaction.internal.jta.JtaStatusHelper;
import org.hibernate.engine.transaction.spi.TransactionContext;
import org.hibernate.engine.transaction.spi.TransactionCoordinator;
Expand All @@ -36,31 +35,43 @@
import org.hibernate.engine.transaction.synchronization.spi.ManagedFlushChecker;
import org.hibernate.engine.transaction.synchronization.spi.SynchronizationCallbackCoordinator;
import org.hibernate.internal.CoreMessageLogger;
import org.jboss.logging.Logger;

/**
* Manages callbacks from the {@link javax.transaction.Synchronization} registered by Hibernate.
*
*
* @author Steve Ebersole
* @author Brett Meyer
*/
public class SynchronizationCallbackCoordinatorImpl implements SynchronizationCallbackCoordinator {

private static final CoreMessageLogger LOG = Logger.getMessageLogger( CoreMessageLogger.class, SynchronizationCallbackCoordinatorImpl.class.getName() );
private static final CoreMessageLogger LOG = Logger.getMessageLogger( CoreMessageLogger.class,
SynchronizationCallbackCoordinatorImpl.class.getName() );

private final TransactionCoordinator transactionCoordinator;
private final Settings settings;

private ManagedFlushChecker managedFlushChecker;
private AfterCompletionAction afterCompletionAction;
private ExceptionMapper exceptionMapper;

private long registrationThreadId;
private final int NO_STATUS = -1;
private int delayedCompletionHandlingStatus;

public SynchronizationCallbackCoordinatorImpl(TransactionCoordinator transactionCoordinator) {
this.transactionCoordinator = transactionCoordinator;
this.settings = transactionCoordinator.getTransactionContext()
.getTransactionEnvironment().getSessionFactory().getSettings();
reset();
pulse();
}

public void reset() {
managedFlushChecker = STANDARD_MANAGED_FLUSH_CHECKER;
exceptionMapper = STANDARD_EXCEPTION_MAPPER;
afterCompletionAction = STANDARD_AFTER_COMPLETION_ACTION;
delayedCompletionHandlingStatus = NO_STATUS;
}

@Override
Expand All @@ -78,24 +89,21 @@ public void setAfterCompletionAction(AfterCompletionAction afterCompletionAction
this.afterCompletionAction = afterCompletionAction;
}


// sync callbacks ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

public void beforeCompletion() {
LOG.trace( "Transaction before completion callback" );

boolean flush;
try {
final int status = transactionCoordinator
.getTransactionContext()
.getTransactionEnvironment()
.getJtaPlatform()
.getCurrentStatus();
final int status = transactionCoordinator.getTransactionContext().getTransactionEnvironment()
.getJtaPlatform().getCurrentStatus();
flush = managedFlushChecker.shouldDoManagedFlush( transactionCoordinator, status );
}
catch ( SystemException se ) {
setRollbackOnly();
throw exceptionMapper.mapStatusCheckFailure( "could not determine transaction status in beforeCompletion()", se );
throw exceptionMapper.mapStatusCheckFailure(
"could not determine transaction status in beforeCompletion()", se );
}

try {
Expand All @@ -119,6 +127,35 @@ private void setRollbackOnly() {
}

public void afterCompletion(int status) {
if ( !settings.isJtaThreadHandling() || isRegistrationThread() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, we should have (re)checked this ;)

Unless I am missing something, don't we miss calling doAfterCompletion (immediately or delayed) when afterCompletion is called with SUCCESS from a "non-main" thread

  1. the thread checks res

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, good catch. Had that originally -- not sure what happened to it. I'll fix and push in a sec.

doAfterCompletion( status );
}
else if ( JtaStatusHelper.isRollback( status ) ) {
// The transaction was rolled back by another thread -- not the
// original application. Examples of this include a JTA transaction
// timeout getting cleaned up by a reaper thread. If this happens,
// afterCompletion must be handled by the original thread in order
// to prevent non-threadsafe session use. Set the flag here and
// check for it in SessionImpl. See HHH-7910.
LOG.warnv( "Transaction afterCompletion called by a background thread! Delaying action until the original thread can handle it. [status={0}]", status );
delayedCompletionHandlingStatus = status;
}
}

public void pulse() {
if ( settings.isJtaThreadHandling() ) {
registrationThreadId = Thread.currentThread().getId();
}
}

public void delayedAfterCompletion() {
if ( delayedCompletionHandlingStatus != NO_STATUS ) {
doAfterCompletion( delayedCompletionHandlingStatus );
delayedCompletionHandlingStatus = NO_STATUS;
}
}

private void doAfterCompletion(int status) {
LOG.tracev( "Transaction after completion callback [status={0}]", status );

try {
Expand All @@ -134,24 +171,29 @@ public void afterCompletion(int status) {
}
}

private boolean isRegistrationThread() {
return Thread.currentThread().getId() == registrationThreadId;
}

private TransactionContext transactionContext() {
return transactionCoordinator.getTransactionContext();
}

private static final ManagedFlushChecker STANDARD_MANAGED_FLUSH_CHECKER = new ManagedFlushChecker() {
@Override
public boolean shouldDoManagedFlush(TransactionCoordinator coordinator, int jtaStatus) {
return ! coordinator.getTransactionContext().isClosed() &&
! coordinator.getTransactionContext().isFlushModeNever() &&
coordinator.getTransactionContext().isFlushBeforeCompletionEnabled() &&
! JtaStatusHelper.isRollback( jtaStatus );
return !coordinator.getTransactionContext().isClosed()
&& !coordinator.getTransactionContext().isFlushModeNever()
&& coordinator.getTransactionContext().isFlushBeforeCompletionEnabled()
&& !JtaStatusHelper.isRollback( jtaStatus );
}
};

private static final ExceptionMapper STANDARD_EXCEPTION_MAPPER = new ExceptionMapper() {
public RuntimeException mapStatusCheckFailure(String message, SystemException systemException) {
LOG.error( LOG.unableToDetermineTransactionStatus(), systemException );
return new TransactionException( "could not determine transaction status in beforeCompletion()", systemException );
return new TransactionException( "could not determine transaction status in beforeCompletion()",
systemException );
}

public RuntimeException mapManagedFlushFailure(String message, RuntimeException failure) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,7 @@
public interface SynchronizationCallbackCoordinator extends Synchronization{
public void setManagedFlushChecker(ManagedFlushChecker managedFlushChecker);
public void setAfterCompletionAction(AfterCompletionAction afterCompletionAction);
public void pulse();
public void delayedAfterCompletion();
public void setExceptionMapper(ExceptionMapper exceptionMapper);
}