TRUNK-6694 & TRUNK-6695: Avoid synchronization in service fetching

Author: ibacher

api/src/main/java/org/openmrs/api/context/ServiceContext.java

@@ -17,6 +17,7 @@
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.aopalliance.aop.Advice;
 import org.openmrs.api.APIException;
@@ -82,7 +83,7 @@ public class ServiceContext implements ApplicationContextAware {
 
 	private ApplicationContext applicationContext;
 
-	private static boolean refreshingContext = false;
+	private static volatile boolean refreshingContext = false;
 
 	private static final Object refreshingContextLock = new Object();
 
@@ -93,7 +94,7 @@ public class ServiceContext implements ApplicationContextAware {
 	private boolean useSystemClassLoader = false;
 
 	// Cached service objects
-	Map<Class, Object> services = new HashMap<>();
+	Map<Class, Object> services = new ConcurrentHashMap<>();
 
 	// Advisors added to services by this service
 	Map<Class, Set<Advisor>> addedAdvisors = new HashMap<>();
@@ -680,20 +681,23 @@ public <T> T getService(Class<? extends T> cls) {
 			log.trace("Getting service: " + cls);
 		}
 
-		// if the context is refreshing, wait until it is
-		// done -- otherwise a null service might be returned
-		synchronized (refreshingContextLock) {
-			try {
-				while (refreshingContext) {
-					log.debug("Waiting to get service: {} while the context is being refreshed", cls);
+		// Only synchronize if we're refreshing the context; during context refreshes, we need
+		// to wait for services to be available. Otherwise we can take the fast-path.
+		if (refreshingContext) {
+			synchronized (refreshingContextLock) {
+				try {
+					while (refreshingContext) {
+						log.debug("Waiting to get service: {} while the context is being refreshed", cls);
 
-					refreshingContextLock.wait();
+						refreshingContextLock.wait();
 
-					log.debug("Finished waiting to get service {} while the context was being refreshed", cls);
-				}
+						log.debug("Finished waiting to get service {} while the context was being refreshed", cls);
+					}
 
-			} catch (InterruptedException e) {
-				log.warn("Refresh lock was interrupted", e);
+				} catch (InterruptedException e) {
+					log.warn("Refresh lock was interrupted", e);
+					Thread.currentThread().interrupt();
+				}
 			}
 		}
 

api/src/test/java/org/openmrs/api/context/ServiceContextTest.java

@@ -11,15 +11,20 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.openmrs.api.APIException;
+import org.openmrs.api.PatientService;
 import org.openmrs.test.jupiter.BaseContextSensitiveTest;
 import org.openmrs.util.DatabaseUpdateException;
 import org.openmrs.util.InputRequiredException;
 
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -102,4 +107,86 @@ public void getModuleOpenmrsServices_shouldRaiseApiExceptionWithNullClassInstanc
 		verify(spiedServiceContext, never()).getMessageService();
 		verify(spiedServiceContext, never()).getMessageSourceService();
 	}
+
+	/**
+	 * @see ServiceContext#getService(Class)
+	 */
+	@Test
+	public void getService_shouldReturnServiceOnTheFastPathWhenNoRefreshIsInProgress() {
+		assertFalse(serviceContext.isRefreshingContext());
+
+		assertNotNull(serviceContext.getService(PatientService.class));
+	}
+
+	/**
+	 * @see ServiceContext#getService(Class)
+	 */
+	@Test
+	public void getService_shouldBlockUntilAnInProgressRefreshCompletes() throws Exception {
+		AtomicReference<Object> retrievedService = new AtomicReference<>();
+		AtomicReference<Throwable> thrown = new AtomicReference<>();
+		CountDownLatch lookupStarted = new CountDownLatch(1);
+
+		serviceContext.startRefreshingContext();
+
+		Thread lookup = new Thread(() -> {
+			lookupStarted.countDown();
+			try {
+				retrievedService.set(serviceContext.getService(PatientService.class));
+			} catch (Throwable t) {
+				thrown.set(t);
+			}
+		});
+		lookup.start();
+
+		try {
+			// wait for the lookup thread to reach getService, then give it time to block
+			assertTrue(lookupStarted.await(5, TimeUnit.SECONDS));
+			Thread.sleep(500);
+
+			// the lookup must not have returned while the refresh is in progress
+			assertNull(retrievedService.get());
+			assertTrue(lookup.isAlive());
+		} finally {
+			serviceContext.doneRefreshingContext();
+		}
+
+		// once the refresh finishes, the blocked lookup should complete
+		lookup.join(5000);
+		assertFalse(lookup.isAlive());
+		assertNull(thrown.get());
+		assertNotNull(retrievedService.get());
+	}
+
+	/**
+	 * @see ServiceContext#setService(Class, Object)
+	 */
+	@Test
+	public void setService_shouldMakeRegistrationsVisibleToOtherThreads() throws Exception {
+		serviceContext.setService(RegistryVisibilityService.class, new RegistryVisibilityServiceImpl());
+
+		AtomicReference<Object> retrievedService = new AtomicReference<>();
+		AtomicReference<Throwable> thrown = new AtomicReference<>();
+
+		Thread reader = new Thread(() -> {
+			try {
+				retrievedService.set(serviceContext.getService(RegistryVisibilityService.class));
+			} catch (Throwable t) {
+				thrown.set(t);
+			}
+		});
+		reader.start();
+		reader.join(5000);
+
+		assertNull(thrown.get());
+		assertNotNull(retrievedService.get());
+		assertTrue(retrievedService.get() instanceof RegistryVisibilityService);
+	}
+
+	/**
+	 * Simple service interface used to verify that registrations are safely published across threads.
+	 */
+	public interface RegistryVisibilityService {}
+
+	public static class RegistryVisibilityServiceImpl implements RegistryVisibilityService {}
 }