Skip to content

Replace synchronized blocks with ReentrantLocks for virtual thread support #2656

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Replace synchronized blocks with ReentrantLocks for virtual thread support #2656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions ...org/springframework/cloud/config/server/environment/AbstractScmEnvironmentRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.stream.Collectors;

import io.micrometer.observation.ObservationRegistry;
Expand All @@ -43,6 +45,8 @@ public abstract class AbstractScmEnvironmentRepository extends AbstractScmAccess

private final ObservationRegistry observationRegistry;

protected final Lock globalLock = new ReentrantLock();

public AbstractScmEnvironmentRepository(ConfigurableEnvironment environment,
ObservationRegistry observationRegistry) {
super(environment);
Expand All @@ -57,22 +61,26 @@ public AbstractScmEnvironmentRepository(ConfigurableEnvironment environment,
}

@Override
public synchronized Environment findOne(String application, String profile, String label) {
public Environment findOne(String application, String profile, String label) {
return findOne(application, profile, label, false);
}

@Override
public synchronized Environment findOne(String application, String profile, String label, boolean includeOrigin) {
var environment = new Environment(application, StringUtils.commaDelimitedListToStringArray(profile), label, "",
"");

for (String l : splitAndReorder(label)) {
var e = findOneInternal(application, profile, l, includeOrigin);
environment.addAll(e.getPropertySources());
environment.setVersion(concat(e.getVersion(), environment.getVersion()));
public Environment findOne(String application, String profile, String label, boolean includeOrigin) {
try {
globalLock.lock();
var environment = new Environment(application, StringUtils.commaDelimitedListToStringArray(profile), label, "",
"");
for (String l : splitAndReorder(label)) {
var e = findOneInternal(application, profile, l, includeOrigin);
environment.addAll(e.getPropertySources());
environment.setVersion(concat(e.getVersion(), environment.getVersion()));
}
return this.cleaner.clean(environment, getWorkingDirectory().toURI().toString(), getUri());
}
finally {
globalLock.unlock();
}

return this.cleaner.clean(environment, getWorkingDirectory().toURI().toString(), getUri());
}

private Environment findOneInternal(String application, String profile, String label, boolean includeOrigin) {
Expand Down
78 changes: 48 additions & 30 deletions ...n/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,34 +253,46 @@ public void setSkipSslValidation(boolean skipSslValidation) {
}

@Override
public synchronized Locations getLocations(String application, String profile, String label) {
if (label == null) {
label = this.defaultLabel;
}
String version;
public Locations getLocations(String application, String profile, String label) {
try {
version = refresh(label);
}
catch (Exception e) {
if (this.defaultLabel.equals(label) && JGitEnvironmentProperties.MAIN_LABEL.equals(this.defaultLabel)
&& tryMasterBranch) {
logger.info("Could not refresh default label " + label, e);
logger.info("Will try to refresh master label instead.");
version = refresh(JGitEnvironmentProperties.MASTER_LABEL);
this.globalLock.lock();
if (label == null) {
label = this.defaultLabel;
}
else {
throw e;
String version;
try {
version = refresh(label);
}
catch (Exception e) {
if (this.defaultLabel.equals(label) && JGitEnvironmentProperties.MAIN_LABEL.equals(this.defaultLabel)
&& tryMasterBranch) {
logger.info("Could not refresh default label " + label, e);
logger.info("Will try to refresh master label instead.");
version = refresh(JGitEnvironmentProperties.MASTER_LABEL);
}
else {
throw e;
}
}
return new Locations(application, profile, label, version,
getSearchLocations(getWorkingDirectory(), application, profile, label));
}
finally {
this.globalLock.unlock();
}
return new Locations(application, profile, label, version,
getSearchLocations(getWorkingDirectory(), application, profile, label));
}

@Override
public synchronized void afterPropertiesSet() throws Exception {
Assert.state(getUri() != null, MESSAGE);
if (this.cloneOnStart) {
initClonedRepository();
public void afterPropertiesSet() throws Exception {
try {
this.globalLock.lock();
Assert.state(getUri() != null, MESSAGE);
if (this.cloneOnStart) {
initClonedRepository();
}
}
finally {
this.globalLock.unlock();
}
}

Expand Down Expand Up @@ -617,20 +629,26 @@ private Git createGitClient() throws IOException, GitAPIException {
}
}

// Synchronize here so that multiple requests don't all try and delete the
// Lock here so that multiple requests don't all try and delete the
// base dir
// together (this is a once only operation, so it only holds things up on
// the first
// request).
private synchronized Git copyRepository() throws IOException, GitAPIException {
deleteBaseDirIfExists();
getBasedir().mkdirs();
Assert.state(getBasedir().exists(), "Could not create basedir: " + getBasedir());
if (getUri().startsWith(FILE_URI_PREFIX)) {
return copyFromLocalRepository();
private Git copyRepository() throws IOException, GitAPIException {
try {
this.globalLock.lock();
deleteBaseDirIfExists();
getBasedir().mkdirs();
Assert.state(getBasedir().exists(), "Could not create basedir: " + getBasedir());
if (getUri().startsWith(FILE_URI_PREFIX)) {
return copyFromLocalRepository();
}
else {
return cloneToBasedir();
}
}
else {
return cloneToBasedir();
finally {
this.globalLock.unlock();
}
}

Expand Down
18 changes: 10 additions & 8 deletions ...java/org/springframework/cloud/config/server/environment/SvnKitEnvironmentRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,17 @@ public void setDefaultLabel(String defaultLabel) {
}

@Override
public synchronized Locations getLocations(String application, String profile, String label) {
if (label == null) {
label = this.defaultLabel;
}
public Locations getLocations(String application, String profile, String label) {
SvnOperationFactory svnOperationFactory = new SvnOperationFactory();
if (hasText(getUsername())) {
svnOperationFactory.setAuthenticationManager(
new DefaultSVNAuthenticationManager(null, false, getUsername(), getPassword()));
}
try {
this.globalLock.lock();
if (label == null) {
label = this.defaultLabel;
}
if (hasText(getUsername())) {
svnOperationFactory.setAuthenticationManager(
new DefaultSVNAuthenticationManager(null, false, getUsername(), getPassword()));
}
String version;
if (new File(getWorkingDirectory(), ".svn").exists()) {
version = update(svnOperationFactory, label);
Expand All @@ -93,6 +94,7 @@ public synchronized Locations getLocations(String application, String profile, S
throw new IllegalStateException("Cannot checkout repository", e);
}
finally {
this.globalLock.unlock();
svnOperationFactory.dispose();
}
}
Expand Down
57 changes: 33 additions & 24 deletions ...main/java/org/springframework/cloud/config/server/resource/GenericResourceRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.Set;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import org.springframework.cloud.config.server.config.ConfigServerProperties;
import org.springframework.cloud.config.server.environment.SearchPathLocator;
Expand All @@ -46,6 +48,8 @@ public class GenericResourceRepository implements ResourceRepository, ResourceLo

private ConfigServerProperties properties;

private final Lock resourceLock = new ReentrantLock();

public GenericResourceRepository(SearchPathLocator service) {
this.service = service;
}
Expand All @@ -61,38 +65,43 @@ public void setResourceLoader(ResourceLoader resourceLoader) {
}

@Override
public synchronized Resource findOne(String application, String profile, String label, String path) {

if (StringUtils.hasText(path)) {
String[] locations = this.service.getLocations(application, profile, label).getLocations();
if (!ObjectUtils.isEmpty(properties) && properties.isReverseLocationOrder()) {
Collections.reverse(Arrays.asList(locations));
}
ArrayList<Resource> locationResources = new ArrayList<>();
for (String location : locations) {
if (!PathUtils.isInvalidEncodedLocation(location)) {
locationResources.add(this.resourceLoader.getResource(location.replaceFirst("optional:", "")));
public Resource findOne(String application, String profile, String label, String path) {
try {
this.resourceLock.lock();
if (StringUtils.hasText(path)) {
String[] locations = this.service.getLocations(application, profile, label).getLocations();
if (!ObjectUtils.isEmpty(properties) && properties.isReverseLocationOrder()) {
Collections.reverse(Arrays.asList(locations));
}
ArrayList<Resource> locationResources = new ArrayList<>();
for (String location : locations) {
if (!PathUtils.isInvalidEncodedLocation(location)) {
locationResources.add(this.resourceLoader.getResource(location.replaceFirst("optional:", "")));
}
}
}

try {
for (Resource location : locationResources) {
for (String local : getProfilePaths(profile, path)) {
if (!PathUtils.isInvalidPath(local) && !PathUtils.isInvalidEncodedPath(local)) {
Resource file = location.createRelative(local);
if (file.exists() && file.isReadable()
&& PathUtils.checkResource(file, location, locationResources)) {
return file;
try {
for (Resource location : locationResources) {
for (String local : getProfilePaths(profile, path)) {
if (!PathUtils.isInvalidPath(local) && !PathUtils.isInvalidEncodedPath(local)) {
Resource file = location.createRelative(local);
if (file.exists() && file.isReadable()
&& PathUtils.checkResource(file, location, locationResources)) {
return file;
}
}
}
}
}
catch (IOException e) {
throw new NoSuchResourceException("Error : " + path + ". (" + e.getMessage() + ")");
}
}
catch (IOException e) {
throw new NoSuchResourceException("Error : " + path + ". (" + e.getMessage() + ")");
}
throw new NoSuchResourceException("Not found: " + path);
}
finally {
this.resourceLock.unlock();
}
throw new NoSuchResourceException("Not found: " + path);
}

private Collection<String> getProfilePaths(String profiles, String path) {
Expand Down
Loading
Loading