Skip to content

Commit

Permalink
OGM-1103 Prevent ConcurrentModificationException in EventContextManager
Browse files Browse the repository at this point in the history
When using an OperationCollector and an OperationsQueue in the
EventContextManager, you might have ended up with a
ConcurrentModificationException as the stateHolder map was
modified while iterating over it.
We now initialize all the required elements at the start of the event
cyle to prevent that.
  • Loading branch information
gunnarmorling authored and DavideD committed Jul 13, 2016
1 parent f756781 commit a08a24f
Show file tree
Hide file tree
Showing 6 changed files with 201 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,19 @@

import org.hibernate.engine.spi.SessionImplementor;
import org.hibernate.event.spi.EventSource;
import org.hibernate.ogm.cfg.OgmProperties;
import org.hibernate.ogm.dialect.impl.BatchOperationsDelegator;
import org.hibernate.ogm.dialect.impl.GridDialects;
import org.hibernate.ogm.dialect.spi.GridDialect;
import org.hibernate.ogm.util.impl.EffectivelyFinal;
import org.hibernate.ogm.util.impl.Immutable;
import org.hibernate.service.Service;
import org.hibernate.service.spi.SessionFactoryServiceRegistry;
import org.hibernate.service.spi.ServiceRegistryAwareService;
import org.hibernate.service.spi.ServiceRegistryImplementor;

/**
* A service which provides access to state specific to one event cycle (currently (auto)-flush or persist).
* <p>
* Client code (such as persisters, dialects etc.) may use this service to propagate state amongst each other, as long
* as they are within the same event cycle. States are identified by class objects which are used as key when accessing
* the contextual map. If a given state type is accessed for the first time during an event cycle, its associated
* {@link EventStateLifecycle} will be invoked to obtain a new instance of that state type.
* the contextual map. At event cycle begin, all enabled {@link EventStateLifecycle}s will be invoked to obtain a new
* instance of their state type.
* <p>
* Accessing the context when not being within the scope of a supported event cycle is illegal.
* <p>
Expand All @@ -36,31 +34,39 @@
*
* @author Gunnar Morling
*/
public class EventContextManager implements Service {
public class EventContextManager implements Service, ServiceRegistryAwareService {

private final ThreadLocal<Map<Class<?>, Object>> stateHolder;

@Immutable
private final Map<Class<?>, EventStateLifecycle<?>> lifecycles;
@EffectivelyFinal
private Map<Class<?>, EventStateLifecycle<?>> enabledLifecycles;

public EventContextManager() {
this.stateHolder = new ThreadLocal<>();
this.lifecycles = Collections.unmodifiableMap( EventStateLifecycles.getLifecycles() );
}

@Override
public void injectServices(ServiceRegistryImplementor serviceRegistry) {
this.enabledLifecycles = Collections.unmodifiableMap( EventStateLifecycles.INSTANCE.getEnabledLifecycles( serviceRegistry ) );
}

/**
* Whether any components will make use of the event context or not.
*/
public static boolean isEventContextRequired(Map<Object, Object> settings, SessionFactoryServiceRegistry serviceRegistry) {
GridDialect gridDialect = serviceRegistry.getService( GridDialect.class );
BatchOperationsDelegator batchDelegator = GridDialects.getDelegateOrNull( gridDialect, BatchOperationsDelegator.class );

return settings.get( OgmProperties.ERROR_HANDLER ) != null || batchDelegator != null;
public static boolean isEventContextRequired(ServiceRegistryImplementor serviceRegistry) {
return !EventStateLifecycles.INSTANCE.getEnabledLifecycles( serviceRegistry ).isEmpty();
}

void onEventBegin(EventSource session) {
Map<Class<?>, Object> stateMap = new HashMap<>();
stateMap.put( SessionImplementor.class, session );

for ( Entry<Class<?>, EventStateLifecycle<?>> lifecycle : enabledLifecycles.entrySet() ) {
Object value = lifecycle.getValue().create( session );
stateMap.put( lifecycle.getKey(), value );
}

stateHolder.set( stateMap );
}

Expand Down Expand Up @@ -98,8 +104,7 @@ public <T> T get(Class<T> stateType) {
T value = getState( states, stateType );

if ( value == null ) {
value = create( stateType, states );
states.put( stateType, value );
throw new IllegalArgumentException( "Accessing state of type not enabled: " + stateType );
}

return value;
Expand All @@ -113,17 +118,6 @@ public boolean isActive() {
return stateHolder.get() != null;
}

private <T> T create(Class<T> stateType, Map<Class<?>, Object> states) {
EventStateLifecycle<T> lifeycle = getLifecycle( stateType );

if ( lifeycle == null ) {
throw new IllegalStateException( "No lifecycle found for state type: " + stateType );
}

SessionImplementor session = getState( states, SessionImplementor.class );
return lifeycle.create( session );
}

private Map<Class<?>, Object> getStates() {
Map<Class<?>, Object> states = stateHolder.get();

Expand All @@ -142,7 +136,7 @@ private <T> T getState(Map<Class<?>, Object> states, Class<T> stateType) {

private <T> EventStateLifecycle<T> getLifecycle(Class<T> stateType) {
@SuppressWarnings("unchecked")
EventStateLifecycle<T> lifecycle = (EventStateLifecycle<T>) lifecycles.get( stateType );
EventStateLifecycle<T> lifecycle = (EventStateLifecycle<T>) enabledLifecycles.get( stateType );
return lifecycle;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
package org.hibernate.ogm.dialect.eventstate.impl;

import org.hibernate.engine.spi.SessionImplementor;
import org.hibernate.service.spi.ServiceRegistryImplementor;

/**
* Callback for event cycle scoped state objects.
Expand All @@ -19,8 +20,13 @@
public interface EventStateLifecycle<T> {

/**
* Creates a new instance of the represented event state type. Invoked by {@link EventContextManager} in case a
* event state type is accessed for the first time during a given event cycle.
* Whether this lifecycle is needed as per the given configuration or not.
*/
boolean mustBeEnabled(ServiceRegistryImplementor serviceRegistry);

/**
* Creates a new instance of the represented event state type. Invoked by {@link EventContextManager} when
* initializing the state context for a given event cycle.
*/
T create(SessionImplementor session);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,22 @@
*/
package org.hibernate.ogm.dialect.eventstate.impl;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Map.Entry;

import org.hibernate.engine.config.spi.ConfigurationService;
import org.hibernate.engine.spi.SessionImplementor;
import org.hibernate.ogm.cfg.OgmProperties;
import org.hibernate.ogm.compensation.impl.ErrorHandlerEnabledTransactionCoordinatorDecorator;
import org.hibernate.ogm.compensation.impl.OperationCollector;
import org.hibernate.ogm.dialect.batch.spi.OperationsQueue;
import org.hibernate.ogm.dialect.impl.BatchOperationsDelegator;
import org.hibernate.ogm.dialect.impl.GridDialects;
import org.hibernate.ogm.dialect.spi.GridDialect;
import org.hibernate.ogm.util.impl.Immutable;
import org.hibernate.service.spi.ServiceRegistryImplementor;

/**
* Holds all known {@link EventStateLifecycle}s.
Expand All @@ -24,16 +30,30 @@
*/
class EventStateLifecycles {

private EventStateLifecycles() {
}
public static final EventStateLifecycles INSTANCE = new EventStateLifecycles();

public static Map<Class<?>, EventStateLifecycle<?>> getLifecycles() {
@Immutable
private final Map<Class<?>, EventStateLifecycle<?>> lifecycles;

private EventStateLifecycles() {
Map<Class<?>, EventStateLifecycle<?>> lifecycles = new HashMap<>();

lifecycles.put( OperationCollector.class, OperationCollectorLifecycle.INSTANCE );
lifecycles.put( OperationsQueue.class, OperationsQueueLifecycle.INSTANCE );

return lifecycles;
this.lifecycles = Collections.unmodifiableMap( lifecycles );
}

public Map<Class<?>, EventStateLifecycle<?>> getEnabledLifecycles(ServiceRegistryImplementor serviceRegistry) {
Map<Class<?>, EventStateLifecycle<?>> enabledLifecycles = new HashMap<>();

for ( Entry<Class<?>, EventStateLifecycle<?>> lifecycle : lifecycles.entrySet() ) {
if ( lifecycle.getValue().mustBeEnabled( serviceRegistry ) ) {
enabledLifecycles.put( lifecycle.getKey(), lifecycle.getValue() );
}
}

return enabledLifecycles;
}

/**
Expand All @@ -43,6 +63,11 @@ private static class OperationCollectorLifecycle implements EventStateLifecycle<

private static EventStateLifecycle<?> INSTANCE = new OperationCollectorLifecycle();

@Override
public boolean mustBeEnabled(ServiceRegistryImplementor serviceRegistry) {
return serviceRegistry.getService( ConfigurationService.class ).getSettings().containsKey( OgmProperties.ERROR_HANDLER );
}

@Override
public OperationCollector create(SessionImplementor session) {
return ( (ErrorHandlerEnabledTransactionCoordinatorDecorator) session.getTransactionCoordinator() ).getOperationCollector();
Expand All @@ -64,6 +89,13 @@ private static class OperationsQueueLifecycle implements EventStateLifecycle<Ope

private static EventStateLifecycle<?> INSTANCE = new OperationsQueueLifecycle();

@Override
public boolean mustBeEnabled(ServiceRegistryImplementor serviceRegistry) {
GridDialect gridDialect = serviceRegistry.getService( GridDialect.class );
BatchOperationsDelegator batchDelegator = GridDialects.getDelegateOrNull( gridDialect, BatchOperationsDelegator.class );
return batchDelegator != null;
}

@Override
public OperationsQueue create(SessionImplementor session) {
return new OperationsQueue();
Expand All @@ -75,7 +107,10 @@ public void onFinish(OperationsQueue operationsQueue, SessionImplementor session
.getServiceRegistry()
.getService( GridDialect.class );

GridDialects.getDelegateOrNull( gridDialect, BatchOperationsDelegator.class ).executeBatch( operationsQueue );
if ( operationsQueue.size() > 0 ) {
GridDialects.getDelegateOrNull( gridDialect, BatchOperationsDelegator.class ).executeBatch( operationsQueue );
}

operationsQueue.close();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@
*/
package org.hibernate.ogm.service.impl;

import java.util.Map;

import org.hibernate.boot.Metadata;
import org.hibernate.engine.config.spi.ConfigurationService;
import org.hibernate.engine.spi.SessionFactoryImplementor;
import org.hibernate.event.service.spi.EventListenerRegistry;
import org.hibernate.event.spi.EventType;
Expand Down Expand Up @@ -58,9 +55,7 @@ private void doIntegrate(Metadata metadata, SessionFactoryImplementor sessionFac
}

private void attachEventContextManagingListenersIfRequired(SessionFactoryServiceRegistry serviceRegistry) {
@SuppressWarnings("unchecked")
Map<Object, Object> settings = serviceRegistry.getService( ConfigurationService.class ).getSettings();
if ( !EventContextManager.isEventContextRequired( settings, serviceRegistry ) ) {
if ( !EventContextManager.isEventContextRequired( serviceRegistry ) ) {
return;
}

Expand All @@ -79,7 +74,6 @@ private void attachEventContextManagingListenersIfRequired(SessionFactoryService
}
}

@SuppressWarnings( "unchecked" )
private <T extends Integrator> T getIntegrator(Class<T> integratorType, SessionFactoryServiceRegistry serviceRegistry) {
Iterable<Integrator> integrators = serviceRegistry.getService( IntegratorService.class ).getIntegrators();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Hibernate OGM, Domain model persistence for NoSQL datastores
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
package org.hibernate.ogm.datastore.mongodb.test.lifecycle;

import javax.persistence.Entity;
import javax.persistence.Id;

import org.bson.types.ObjectId;

/**
* @author Gunnar Morling
*/
@Entity
public class BarKeeper {

private ObjectId id;
private String name;

BarKeeper() {
}

BarKeeper(ObjectId id, String name) {
this.id = id;
this.name = name;
}

@Id
public ObjectId getId() {
return id;
}

public void setId(ObjectId id) {
this.id = id;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Hibernate OGM, Domain model persistence for NoSQL datastores
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
package org.hibernate.ogm.datastore.mongodb.test.lifecycle;

import static org.fest.assertions.Assertions.assertThat;

import java.util.Map;

import org.bson.types.ObjectId;
import org.hibernate.Transaction;
import org.hibernate.cfg.AvailableSettings;
import org.hibernate.ogm.OgmSession;
import org.hibernate.ogm.cfg.OgmProperties;
import org.hibernate.ogm.compensation.ErrorHandler;
import org.hibernate.ogm.compensation.ErrorHandlingStrategy;
import org.hibernate.ogm.utils.OgmTestCase;
import org.hibernate.ogm.utils.TestForIssue;
import org.junit.Test;

/**
* Tests for using object ids with MongoDB.
*
* @author Gunnar Morling
*
*/
public class PersistOperationCollectorTest extends OgmTestCase {

@Test
@TestForIssue(jiraKey = "OGM-1103")
public void noConcurrentModificationExceptionWhenUsingOperationCollector() {
OgmSession session = openSession();
Transaction tx = session.beginTransaction();

// given
BarKeeper brian = new BarKeeper( new ObjectId(), "Brian" );

// when
session.persist( brian );
session.flush();

brian.setName( "Bruce" );
tx.commit();
session.clear();
tx = session.beginTransaction();

BarKeeper brianLoaded = session.load( BarKeeper.class, brian.getId() );

// then
assertThat( brianLoaded.getId() ).isEqualTo( brian.getId() );
assertThat( brianLoaded.getName() ).isEqualTo( "Bruce" );

tx.commit();
session.close();
}

@Override
protected Class<?>[] getAnnotatedClasses() {
return new Class<?>[] { BarKeeper.class };
}

@Override
protected void configure(Map<String, Object> settings) {
settings.put( AvailableSettings.USE_NEW_ID_GENERATOR_MAPPINGS, false );
settings.put( OgmProperties.ERROR_HANDLER, MyErrorHandler.class );
}

public static class MyErrorHandler implements ErrorHandler {

@Override
public ErrorHandlingStrategy onFailedGridDialectOperation(FailedGridDialectOperationContext context) {
return ErrorHandlingStrategy.ABORT;
}

@Override
public void onRollback(RollbackContext context) {
}
}
}

0 comments on commit a08a24f

Please sign in to comment.