Skip to content

Commit

Permalink
5778-app - Fix potential threadLocalTrxName leakage
Browse files Browse the repository at this point in the history
* AbstractTrxManager: store threadLocalTrx in a *normal* ThreadLocal; not in `InheritableThreadLocal`, because to inherit this stuff from elsewhere is exactly what we do *not* want
#5778
  • Loading branch information
metas-ts committed Nov 11, 2019
1 parent fd8e9df commit 5ffa877
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 31 deletions.
Expand Up @@ -339,8 +339,6 @@ default <T> T callInThreadInheritedTrx(@NonNull final TrxCallable<T> callable)
* </ul>
* this method will return a pseudo- {@link ITrxListenerManager} which automatically commits when a listener is registered.
*
*
* @param trxName
* @return auto-commit {@link ITrxListenerManager}; never returns null
*/
ITrxListenerManager getTrxListenerManagerOrAutoCommit(String trxName);
Expand Down
Expand Up @@ -471,7 +471,7 @@ private ITrxListenerManager getTrxListenerManager(final boolean create)

if (create)
{
trxListenerManager = new TrxListenerManager();
trxListenerManager = new TrxListenerManager(getTrxName());
return trxListenerManager;
}

Expand All @@ -489,7 +489,7 @@ public final ITrxManager getTrxManager()
{
return trxManager;
}

private final Map<String, Object> getPropertiesMap()
{
if(_properties == null)
Expand All @@ -504,7 +504,7 @@ private final Map<String, Object> getPropertiesMap()
}
return _properties;
}

private final Map<String, Object> getPropertiesMapOrNull()
{
synchronized (this)
Expand All @@ -517,7 +517,7 @@ private final Map<String, Object> getPropertiesMapOrNull()
public final <T> T setProperty(final String name, final Object value)
{
Check.assumeNotEmpty(name, "name is not empty");

// Handle null value case
if(value == null)
{
Expand All @@ -526,7 +526,7 @@ public final <T> T setProperty(final String name, final Object value)
{
return null;
}

@SuppressWarnings("unchecked")
final T valueOld = (T)properties.remove(name);
return valueOld;
Expand Down Expand Up @@ -554,29 +554,29 @@ public <T> T getProperty(final String name, final Supplier<T> valueInitializer)
final T value = (T)getPropertiesMap().computeIfAbsent(name, key->valueInitializer.get());
return value;
}

@Override
public <T> T getProperty(final String name, final Function<ITrx, T> valueInitializer)
{
@SuppressWarnings("unchecked")
final T value = (T)getPropertiesMap().computeIfAbsent(name, key->valueInitializer.apply(this));
return value;
}

@Override
public <T> T setAndGetProperty(@NonNull final String name, @NonNull final Function<T, T> valueRemappingFunction)
{
final BiFunction<? super String, ? super Object, ? extends Object> remappingFunction = (propertyName, oldValue) -> {
@SuppressWarnings("unchecked")
final T oldValueCasted = (T)oldValue;

return valueRemappingFunction.apply(oldValueCasted);
};

@SuppressWarnings("unchecked")
final T value = (T)getPropertiesMap().compute(name, remappingFunction);
return value;

}


Expand Down
Expand Up @@ -92,11 +92,9 @@ public abstract class AbstractTrxManager implements ITrxManager

private ITrxNameGenerator trxNameGenerator = DefaultTrxNameGenerator.instance;

/**
* Thread level transaction name
*/
private final InheritableThreadLocal<String> threadLocalTrx = new InheritableThreadLocal<>();
private final InheritableThreadLocal<OnRunnableFail> threadLocalOnRunnableFail = new InheritableThreadLocal<>();
/** Name of the trx currently associated with this thread */
private final ThreadLocal<String> threadLocalTrx = new ThreadLocal<>();
private final ThreadLocal<OnRunnableFail> threadLocalOnRunnableFail = new ThreadLocal<>();

private boolean debugTrxCreateStacktrace = false;
private boolean debugTrxCloseStacktrace = false;
Expand All @@ -110,8 +108,6 @@ public abstract class AbstractTrxManager implements ITrxManager

public AbstractTrxManager()
{
super();

final JMXTrxManager jmxBean = new JMXTrxManager(this);
JMXRegistry.get().registerJMX(jmxBean, OnJMXAlreadyExistsPolicy.Replace);
}
Expand Down Expand Up @@ -664,7 +660,7 @@ else if (trxPropagation == TrxPropagation.NESTED)

// Set our transaction as currently active thread local transaction
restoreThreadLocalTrxName = true;
threadLocalTrx.set(trxNameToUse);
setThreadInheritedTrxName(trxNameToUse);

return call0(callable, cfg, trxNameToUse);
}
Expand All @@ -673,7 +669,7 @@ else if (trxPropagation == TrxPropagation.NESTED)
// Restore the thread local transaction, in case we changed it.
if (restoreThreadLocalTrxName)
{
threadLocalTrx.set(threadLocalTrxNameOld);
setThreadInheritedTrxName(threadLocalTrxNameOld);
restoreThreadLocalTrxName = false;
}

Expand Down
Expand Up @@ -42,28 +42,29 @@

/**
* Default {@link ITrxListenerManager} implementation
*
* @author tsa
*
*/
public class TrxListenerManager implements ITrxListenerManager
{
private static final Logger logger = LogManager.getLogger(TrxListenerManager.class);

private volatile WeakList<RegisterListenerRequest> listeners = null;

/** for debugging */
private final String trxName;

/**
* Never contains {@code null} or {@link TrxEventTiming#UNSPECIFIED}.
*/
private final AtomicReference<TrxEventTiming> runningWithinTrxEventTiming = new AtomicReference<>(TrxEventTiming.NONE);

private static enum OnError
private enum OnError
{
ThrowException, LogAndSkip
};
}

public TrxListenerManager()
public TrxListenerManager(final String trxName)
{
this.trxName = trxName;
}

@Override
Expand Down Expand Up @@ -95,20 +96,22 @@ public void registerListener(@NonNull final RegisterListenerRequest listener)
private void verifyEventTiming(@NonNull final RegisterListenerRequest listener)
{
final TrxEventTiming eventTimingOfListener = listener.getTiming();
final boolean listenerHasProblematicTiming = !eventTimingOfListener.canBeRegisteredWithinOtherTiming(runningWithinTrxEventTiming.get());

final TrxEventTiming currentTiming = runningWithinTrxEventTiming.get();
final boolean listenerHasProblematicTiming = !eventTimingOfListener.canBeRegisteredWithinOtherTiming(currentTiming);
if (listenerHasProblematicTiming)
{
final String message = StringUtils.formatMessage("Registering another listener within a listener's event handling code might be a development error and that other listener might not be fired."
+ "\n trxName={}"
+ "\n current trx event timing={}"
+ "\n listener that is registerd={}",
runningWithinTrxEventTiming.get(),
this.trxName,
currentTiming,
listener);

new AdempiereException(message).throwIfDeveloperModeOrLogWarningElse(logger);
}
}

@Override
public boolean canRegisterOnTiming(@NonNull final TrxEventTiming timing)
{
Expand Down

0 comments on commit 5ffa877

Please sign in to comment.