Revise synchronized blocks

- Replace synchronized with Lock when guarding long-running operations
- Remove unnecessary synchronization in FileSystemWatcher
- Replace HashMap with ConcurrentHashMap in Restarter
- Remove unnecessary locking on AtomicBoolean in
  SpringApplicationBuilder
- Remove unnecessary locking in SimpleFormatter

Closes gh-36670
This commit is contained in:
Moritz Halbritter
2023-08-02 14:30:38 +02:00
parent 6506208d29
commit 497bbf9c2d
19 changed files with 380 additions and 150 deletions

View File

@@ -76,7 +76,7 @@ public class SpringApplicationBuilder {
private final SpringApplication application;
private ConfigurableApplicationContext context;
private volatile ConfigurableApplicationContext context;
private SpringApplicationBuilder parent;
@@ -145,10 +145,8 @@ public class SpringApplicationBuilder {
}
configureAsChildIfNecessary(args);
if (this.running.compareAndSet(false, true)) {
synchronized (this.running) {
// If not already running copy the sources over and then run.
this.context = build().run(args);
}
// If not already running copy the sources over and then run.
this.context = build().run(args);
}
return this.context;
}

View File

@@ -29,6 +29,8 @@ import java.util.EnumSet;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.stream.Stream;
import org.springframework.boot.convert.ApplicationConversionService;
@@ -258,6 +260,11 @@ public class ConfigTreePropertySource extends EnumerablePropertySource<Path> imp
private final Path path;
/**
* Guards access to {@link #resource}.
*/
private final Lock resourceLock = new ReentrantLock();
private final Resource resource;
private final Origin origin;
@@ -341,11 +348,15 @@ public class ConfigTreePropertySource extends EnumerablePropertySource<Path> imp
}
if (this.content == null) {
assertStillExists();
synchronized (this.resource) {
this.resourceLock.lock();
try {
if (this.content == null) {
this.content = FileCopyUtils.copyToByteArray(this.resource.getInputStream());
}
}
finally {
this.resourceLock.unlock();
}
}
return this.content;
}

View File

@@ -38,17 +38,15 @@ public class SimpleFormatter extends Formatter {
private final String pid = getOrUseDefault(LoggingSystemProperty.PID.getEnvironmentVariableName(), "????");
private final Date date = new Date();
@Override
public synchronized String format(LogRecord record) {
this.date.setTime(record.getMillis());
public String format(LogRecord record) {
Date date = new Date(record.getMillis());
String source = record.getLoggerName();
String message = formatMessage(record);
String throwable = getThrowable(record);
String thread = getThreadName();
return String.format(this.format, this.date, source, record.getLoggerName(),
record.getLevel().getLocalizedName(), message, throwable, thread, this.pid);
return String.format(this.format, date, source, record.getLoggerName(), record.getLevel().getLocalizedName(),
message, throwable, thread, this.pid);
}
private String getThrowable(LogRecord record) {

View File

@@ -1,5 +1,5 @@
/*
* Copyright 2012-2020 the original author or authors.
* Copyright 2012-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -16,6 +16,8 @@
package org.springframework.boot.security.reactive;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Supplier;
import reactor.core.publisher.Mono;
@@ -44,7 +46,7 @@ public abstract class ApplicationContextServerWebExchangeMatcher<C> implements S
private volatile Supplier<C> context;
private final Object contextLock = new Object();
private final Lock contextLock = new ReentrantLock();
public ApplicationContextServerWebExchangeMatcher(Class<? extends C> contextClass) {
Assert.notNull(contextClass, "Context class must not be null");
@@ -81,13 +83,17 @@ public abstract class ApplicationContextServerWebExchangeMatcher<C> implements S
protected Supplier<C> getContext(ServerWebExchange exchange) {
if (this.context == null) {
synchronized (this.contextLock) {
this.contextLock.lock();
try {
if (this.context == null) {
Supplier<C> createdContext = createContext(exchange);
initialized(createdContext);
this.context = createdContext;
}
}
finally {
this.contextLock.unlock();
}
}
return this.context;
}

View File

@@ -16,6 +16,8 @@
package org.springframework.boot.security.servlet;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Supplier;
import jakarta.servlet.http.HttpServletRequest;
@@ -45,7 +47,7 @@ public abstract class ApplicationContextRequestMatcher<C> implements RequestMatc
private volatile boolean initialized;
private final Object initializeLock = new Object();
private final Lock initializeLock = new ReentrantLock();
public ApplicationContextRequestMatcher(Class<? extends C> contextClass) {
Assert.notNull(contextClass, "Context class must not be null");
@@ -61,12 +63,16 @@ public abstract class ApplicationContextRequestMatcher<C> implements RequestMatc
}
Supplier<C> context = () -> getContext(webApplicationContext);
if (!this.initialized) {
synchronized (this.initializeLock) {
this.initializeLock.lock();
try {
if (!this.initialized) {
initialized(context);
this.initialized = true;
}
}
finally {
this.initializeLock.unlock();
}
}
return matches(request, context);
}

View File

@@ -1,5 +1,5 @@
/*
* Copyright 2012-2022 the original author or authors.
* Copyright 2012-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -28,6 +28,8 @@ import java.nio.file.attribute.PosixFilePermissions;
import java.security.MessageDigest;
import java.util.EnumSet;
import java.util.HexFormat;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import org.springframework.util.Assert;
import org.springframework.util.StringUtils;
@@ -51,6 +53,11 @@ public class ApplicationTemp {
private volatile Path path;
/**
* Guards access to {@link #path}.
*/
private final Lock pathLock = new ReentrantLock();
/**
* Create a new {@link ApplicationTemp} instance.
*/
@@ -90,10 +97,14 @@ public class ApplicationTemp {
private Path getPath() {
if (this.path == null) {
synchronized (this) {
this.pathLock.lock();
try {
String hash = HexFormat.of().withUpperCase().formatHex(generateHash(this.sourceClass));
this.path = createDirectory(getTempDirectory().resolve(hash));
}
finally {
this.pathLock.unlock();
}
}
return this.path;
}

View File

@@ -20,6 +20,8 @@ import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.stream.Collectors;
import org.apache.commons.logging.Log;
@@ -57,7 +59,7 @@ public class JettyWebServer implements WebServer {
private static final Log logger = LogFactory.getLog(JettyWebServer.class);
private final Object monitor = new Object();
private final Lock lock = new ReentrantLock();
private final Server server;
@@ -113,21 +115,23 @@ public class JettyWebServer implements WebServer {
}
private void initialize() {
synchronized (this.monitor) {
try {
// Cache the connectors and then remove them to prevent requests being
// handled before the application context is ready.
this.connectors = this.server.getConnectors();
JettyWebServer.this.server.setConnectors(null);
// Start the server so that the ServletContext is available
this.server.start();
this.server.setStopAtShutdown(false);
}
catch (Throwable ex) {
// Ensure process isn't left running
stopSilently();
throw new WebServerException("Unable to start embedded Jetty web server", ex);
}
this.lock.lock();
try {
// Cache the connectors and then remove them to prevent requests being
// handled before the application context is ready.
this.connectors = this.server.getConnectors();
JettyWebServer.this.server.setConnectors(null);
// Start the server so that the ServletContext is available
this.server.start();
this.server.setStopAtShutdown(false);
}
catch (Throwable ex) {
// Ensure process isn't left running
stopSilently();
throw new WebServerException("Unable to start embedded Jetty web server", ex);
}
finally {
this.lock.unlock();
}
}
@@ -142,7 +146,8 @@ public class JettyWebServer implements WebServer {
@Override
public void start() throws WebServerException {
synchronized (this.monitor) {
this.lock.lock();
try {
if (this.started) {
return;
}
@@ -179,6 +184,9 @@ public class JettyWebServer implements WebServer {
throw new WebServerException("Unable to start embedded Jetty server", ex);
}
}
finally {
this.lock.unlock();
}
}
String getStartedLogMessage() {
@@ -241,7 +249,8 @@ public class JettyWebServer implements WebServer {
@Override
public void stop() {
synchronized (this.monitor) {
this.lock.lock();
try {
this.started = false;
if (this.gracefulShutdown != null) {
this.gracefulShutdown.abort();
@@ -258,17 +267,22 @@ public class JettyWebServer implements WebServer {
throw new WebServerException("Unable to stop embedded Jetty server", ex);
}
}
finally {
this.lock.unlock();
}
}
@Override
public void destroy() {
synchronized (this.monitor) {
try {
this.server.stop();
}
catch (Exception ex) {
throw new WebServerException("Unable to destroy embedded Jetty server", ex);
}
this.lock.lock();
try {
this.server.stop();
}
catch (Exception ex) {
throw new WebServerException("Unable to destroy embedded Jetty server", ex);
}
finally {
this.lock.unlock();
}
}

View File

@@ -20,6 +20,8 @@ import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.stream.Collectors;
import javax.naming.NamingException;
@@ -60,7 +62,7 @@ public class TomcatWebServer implements WebServer {
private static final AtomicInteger containerCounter = new AtomicInteger(-1);
private final Object monitor = new Object();
private final Lock lock = new ReentrantLock();
private final Map<Service, Connector[]> serviceConnectors = new HashMap<>();
@@ -106,41 +108,43 @@ public class TomcatWebServer implements WebServer {
private void initialize() throws WebServerException {
logger.info("Tomcat initialized with " + getPortsDescription(false));
synchronized (this.monitor) {
this.lock.lock();
try {
addInstanceIdToEngineName();
Context context = findContext();
context.addLifecycleListener((event) -> {
if (context.equals(event.getSource()) && Lifecycle.START_EVENT.equals(event.getType())) {
// Remove service connectors so that protocol binding doesn't
// happen when the service is started.
removeServiceConnectors();
}
});
// Start the server to trigger initialization listeners
this.tomcat.start();
// We can re-throw failure exception directly in the main thread
rethrowDeferredStartupExceptions();
try {
addInstanceIdToEngineName();
Context context = findContext();
context.addLifecycleListener((event) -> {
if (context.equals(event.getSource()) && Lifecycle.START_EVENT.equals(event.getType())) {
// Remove service connectors so that protocol binding doesn't
// happen when the service is started.
removeServiceConnectors();
}
});
// Start the server to trigger initialization listeners
this.tomcat.start();
// We can re-throw failure exception directly in the main thread
rethrowDeferredStartupExceptions();
try {
ContextBindings.bindClassLoader(context, context.getNamingToken(), getClass().getClassLoader());
}
catch (NamingException ex) {
// Naming is not enabled. Continue
}
// Unlike Jetty, all Tomcat threads are daemon threads. We create a
// blocking non-daemon to stop immediate shutdown
startDaemonAwaitThread();
ContextBindings.bindClassLoader(context, context.getNamingToken(), getClass().getClassLoader());
}
catch (Exception ex) {
stopSilently();
destroySilently();
throw new WebServerException("Unable to start embedded Tomcat", ex);
catch (NamingException ex) {
// Naming is not enabled. Continue
}
// Unlike Jetty, all Tomcat threads are daemon threads. We create a
// blocking non-daemon to stop immediate shutdown
startDaemonAwaitThread();
}
catch (Exception ex) {
stopSilently();
destroySilently();
throw new WebServerException("Unable to start embedded Tomcat", ex);
}
finally {
this.lock.unlock();
}
}
@@ -205,7 +209,8 @@ public class TomcatWebServer implements WebServer {
@Override
public void start() throws WebServerException {
synchronized (this.monitor) {
this.lock.lock();
try {
if (this.started) {
return;
}
@@ -233,6 +238,9 @@ public class TomcatWebServer implements WebServer {
ContextBindings.unbindClassLoader(context, context.getNamingToken(), getClass().getClassLoader());
}
}
finally {
this.lock.unlock();
}
}
String getStartedLogMessage() {
@@ -324,7 +332,8 @@ public class TomcatWebServer implements WebServer {
@Override
public void stop() throws WebServerException {
synchronized (this.monitor) {
this.lock.lock();
try {
boolean wasStarted = this.started;
try {
this.started = false;
@@ -342,6 +351,9 @@ public class TomcatWebServer implements WebServer {
}
}
}
finally {
this.lock.unlock();
}
}
@Override

View File

@@ -25,6 +25,8 @@ import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import io.undertow.Undertow;
import io.undertow.server.HttpHandler;
@@ -63,7 +65,7 @@ public class UndertowWebServer implements WebServer {
private final AtomicReference<GracefulShutdownCallback> gracefulShutdownCallback = new AtomicReference<>();
private final Object monitor = new Object();
private final Lock lock = new ReentrantLock();
private final Undertow.Builder builder;
@@ -104,7 +106,8 @@ public class UndertowWebServer implements WebServer {
@Override
public void start() throws WebServerException {
synchronized (this.monitor) {
this.lock.lock();
try {
if (this.started) {
return;
}
@@ -136,6 +139,9 @@ public class UndertowWebServer implements WebServer {
}
}
}
finally {
this.lock.unlock();
}
}
private void destroySilently() {
@@ -268,7 +274,8 @@ public class UndertowWebServer implements WebServer {
@Override
public void stop() throws WebServerException {
synchronized (this.monitor) {
this.lock.lock();
try {
if (!this.started) {
return;
}
@@ -286,6 +293,9 @@ public class UndertowWebServer implements WebServer {
throw new WebServerException("Unable to stop Undertow", ex);
}
}
finally {
this.lock.unlock();
}
}
@Override