From c7e74afc583e8966150247efff772c742847f2c6 Mon Sep 17 00:00:00 2001 From: Henri Tremblay Date: Thu, 5 Jun 2025 13:26:47 -0400 Subject: [PATCH 1/7] Use never() instead of times(0) Signed-off-by: Henri Tremblay --- .../JGitEnvironmentRepositoryTests.java | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java index 30d4e5ee8b..04565cefff 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java @@ -87,6 +87,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockingDetails; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -285,8 +286,8 @@ public void afterPropertiesSet_CloneOnStartFalse_CloneAndFetchNotCalled() throws envRepository.setGitFactory(new MockGitFactory(mockGit, mockCloneCommand)); envRepository.setUri("http://somegitserver/somegitrepo"); envRepository.afterPropertiesSet(); - verify(mockCloneCommand, times(0)).call(); - verify(mockGit, times(0)).fetch(); + verify(mockCloneCommand, never()).call(); + verify(mockGit, never()).fetch(); } @Test @@ -303,8 +304,8 @@ public void afterPropertiesSet_CloneOnStartTrueWithFileURL_CloneAndFetchNotCalle envRepository.setUri("file://somefilesystem/somegitrepo"); envRepository.setCloneOnStart(true); envRepository.afterPropertiesSet(); - verify(mockCloneCommand, times(0)).call(); - verify(mockGit, times(0)).fetch(); + verify(mockCloneCommand, never()).call(); + verify(mockGit, never()).fetch(); } @Test @@ -638,7 +639,7 @@ public void testFetchException() throws Exception { SearchPathLocator.Locations locations = this.repository.getLocations("bar", "staging", null); assertThat(newObjectId.getName()).isEqualTo(locations.getVersion()); - verify(git, times(0)).branchDelete(); + verify(git, never()).branchDelete(); } private Repository stubbedRepo() { @@ -753,7 +754,7 @@ public void testMergeException() throws Exception { SearchPathLocator.Locations locations = this.repository.getLocations("bar", "staging", "master"); assertThat(newObjectId.getName()).isEqualTo(locations.getVersion()); - verify(git, times(0)).branchDelete(); + verify(git, never()).branchDelete(); } @Test @@ -806,8 +807,8 @@ public void testRefreshWithoutFetch() throws Exception { repo.refresh("master"); - // Verify no fetch but merge only. - verify(git, times(0)).fetch(); + // Verify no fetch nor merge. + verify(git, never()).fetch(); verify(git).merge(); } @@ -865,7 +866,7 @@ public void testResetHardException() throws Exception { SearchPathLocator.Locations locations = this.repository.getLocations("bar", "staging", "master"); assertThat(newObjectId.getName()).isEqualTo(locations.getVersion()); - verify(git, times(0)).branchDelete(); + verify(git, never()).branchDelete(); } @Test @@ -1425,9 +1426,9 @@ public void afterPropertiesSet_CloneOnStartTrue_DefaultLabelSameAsDefaultBranch_ envRepository.afterPropertiesSet(); verify(mockCloneCommand, times(1)).call(); // Checkout/List Branch/Checkout setName should not be called - verify(mockCheckoutCommand, times(0)).call(); - verify(mockListBranchCommand, times(0)).call(); - verify(mockCheckoutCommand, times(0)).setName(anyString()); + verify(mockCheckoutCommand, never()).call(); + verify(mockListBranchCommand, never()).call(); + verify(mockCheckoutCommand, never()).setName(anyString()); } class MockCloneCommand extends CloneCommand { From 50970d64a23597adfe8cde7348b8a23fa675e15a Mon Sep 17 00:00:00 2001 From: Henri Tremblay Date: Thu, 5 Jun 2025 13:27:31 -0400 Subject: [PATCH 2/7] Verify the checkout on top of the merge to be more precise Signed-off-by: Henri Tremblay --- .../server/environment/JGitEnvironmentRepositoryTests.java | 1 + 1 file changed, 1 insertion(+) diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java index 04565cefff..060bb0fd85 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java @@ -809,6 +809,7 @@ public void testRefreshWithoutFetch() throws Exception { // Verify no fetch nor merge. verify(git, never()).fetch(); + verify(git).checkout(); verify(git).merge(); } From 523e95e3d2a9817fef4045dea003118a9e02ce88 Mon Sep 17 00:00:00 2001 From: Henri Tremblay Date: Thu, 5 Jun 2025 14:46:06 -0400 Subject: [PATCH 3/7] Since we are locking above, it makes sense to lock in the exception. And we better use a ReentrantLock I think. At least it will prevent virtual thread pinning Signed-off-by: Henri Tremblay --- .../JGitEnvironmentRepository.java | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java index 66c34b9f50..b0e9f67d48 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java @@ -24,6 +24,7 @@ import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.concurrent.locks.ReentrantLock; import io.micrometer.observation.ObservationRegistry; import org.eclipse.jgit.api.CheckoutCommand; @@ -155,7 +156,7 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository * both the ResourceController and the EnvironmentController. See #2681. */ - private final Object LOCK = new Object(); + private final ReentrantLock LOCK = new ReentrantLock(); public JGitEnvironmentRepository(ConfigurableEnvironment environment, JGitEnvironmentProperties properties, ObservationRegistry observationRegistry) { @@ -262,9 +263,13 @@ public void setSkipSslValidation(boolean skipSslValidation) { @Override public synchronized Environment findOne(String application, String profile, String label, boolean includeOrigin) { - synchronized (LOCK) { + LOCK.lock(); + try { return super.findOne(application, profile, label, includeOrigin); } + finally { + LOCK.unlock(); + } } @Override @@ -274,16 +279,26 @@ public synchronized Locations getLocations(String application, String profile, S } String version; try { - synchronized (LOCK) { + try { + LOCK.lock(); version = refresh(label); } + finally { + LOCK.unlock(); + } } 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); + LOCK.lock(); + try { + version = refresh(JGitEnvironmentProperties.MASTER_LABEL); + } + finally { + LOCK.unlock(); + } } else { throw e; From f5ea586a7c64b2a3186ebc2c12140f18d2e51274 Mon Sep 17 00:00:00 2001 From: Henri Tremblay Date: Thu, 5 Jun 2025 14:46:27 -0400 Subject: [PATCH 4/7] Make it a long right away to prevent implicit casting Signed-off-by: Henri Tremblay --- .../config/server/environment/JGitEnvironmentRepository.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java index b0e9f67d48..012403843d 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java @@ -503,7 +503,7 @@ protected boolean shouldPull(Git git) throws GitAPIException { boolean shouldPull; if (this.refreshRate < 0 || (this.refreshRate > 0 - && System.currentTimeMillis() - this.lastRefresh < (this.refreshRate * 1000))) { + && System.currentTimeMillis() - this.lastRefresh < (this.refreshRate * 1000L))) { return false; } From 1ef34990a01198f4c892a4484fae6aaffca920c7 Mon Sep 17 00:00:00 2001 From: Henri Tremblay Date: Thu, 5 Jun 2025 23:20:23 -0400 Subject: [PATCH 5/7] Use isEmpty instead Signed-off-by: Henri Tremblay --- .../config/server/environment/JGitEnvironmentRepository.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java index 012403843d..801273ba86 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java @@ -581,7 +581,7 @@ protected FetchResult fetch(Git git, String label) { configureCommand(fetch); try { FetchResult result = fetch.call(); - if (result.getTrackingRefUpdates() != null && result.getTrackingRefUpdates().size() > 0) { + if (result.getTrackingRefUpdates() != null && !result.getTrackingRefUpdates().isEmpty()) { this.logger.info("Fetched for remote " + label + " and found " + result.getTrackingRefUpdates().size() + " updates"); } From 14e2169d38d03bb1697800c8bc4ee8c6e937cc41 Mon Sep 17 00:00:00 2001 From: Henri Tremblay Date: Thu, 5 Jun 2025 23:21:41 -0400 Subject: [PATCH 6/7] Performance improvement by doing a checkout and merge only when the label is changing or if it's refresh time Signed-off-by: Henri Tremblay --- .../JGitEnvironmentRepository.java | 34 +++++++++++++++---- ...EnvironmentRepositoryIntegrationTests.java | 13 +++++-- .../JGitEnvironmentRepositoryTests.java | 27 ++++++++++++--- 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java index 801273ba86..d2c0970c4b 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java @@ -151,6 +151,15 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository private final ObservationRegistry observationRegistry; + /** + * Contains the previousLabel that was checked out to prevent doing it again. + * + * @implNote + * The field is not volatile since we tend to lock around the {@link #refresh(String)} method which uses it. + * However, this method is public si maybe it's a bad bet. + */ + private String previousLabel = "$%^&"; // none existing branch + /** * This lock is used to ensure thread safety between accessing the local git repo from * both the ResourceController and the EnvironmentController. See we are on the same label, we should do nothing + // case b -> we should checkout if (shouldPull(git)) { FetchResult fetchStatus = fetch(git, label); if (this.deleteUntrackedBranches && fetchStatus != null) { deleteUntrackedLocalBranches(fetchStatus.getTrackingRefUpdates(), git); } - } - // checkout after fetch so we can get any new branches, tags, ect. - // if nothing to update so just checkout and merge. - // Merge because remote branch could have been updated before - checkout(git, label); - tryMerge(git, label); + // checkout after fetch, so we can get any new branches, tags, etc. + checkout(git, label); + // merge because remote branch could have been updated + tryMerge(git, label); + } + else if (!label.equals(this.previousLabel)) { + // checkout the new label + checkout(git, label); + } // always return what is currently HEAD as the version return git.getRepository().findRef("HEAD").getObjectId().getName(); @@ -496,7 +514,9 @@ private Ref checkout(Git git, String label) throws GitAPIException { // works for tags and local branches checkout.setName(label); } - return checkout.call(); + Ref call = checkout.call(); + previousLabel = label; // do it after the call to prevent changing it when it fails + return call; } protected boolean shouldPull(Git git) throws GitAPIException { diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java index a898315f4e..f64e78e68e 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java @@ -533,12 +533,13 @@ public void testNewCommitIDWithRefreshRate() throws Exception { StreamUtils.copy("foo: barNewCommit", Charset.defaultCharset(), out); testData.getServerGit().getGit().add().addFilepattern("bar.properties").call(); testData.getServerGit().getGit().commit().setMessage("Updated for pull").call(); + testData.getServerGit().getGit().tag().setName("testTag").call(); String updatedRemoteVersion = getCommitID(testData.getServerGit().getGit(), "master"); // Set refresh rate to 60 seconds (now it will fetch the remote repo only once) testData.getRepository().setRefreshRate(60); - // Ask test label configuration first + // Ask test label configuration first, it will fetch but fail on the checkout try { testData.getRepository().findOne("bar", "staging", "test"); fail("Should have thrown NoSuchLabelException."); @@ -547,10 +548,16 @@ public void testNewCommitIDWithRefreshRate() throws Exception { // OK } - // do a normal request and verify we get the new version + // do a normal request, the previous branch was the "master" branch, so it won't move environment = testData.getRepository().findOne("bar", "staging", "master"); - assertThat(updatedRemoteVersion).isEqualTo(environment.getVersion()); + assertThat(startingRemoteVersion).isEqualTo(environment.getVersion()); Object fooProperty = ConfigServerTestUtils.getProperty(environment, "bar.properties", "foo"); + assertThat("bar").isEqualTo(fooProperty); + + // get the test tag which is on the latest commit. It was fetched so it will be found + environment = testData.getRepository().findOne("bar", "staging", "testTag"); + assertThat(updatedRemoteVersion).isEqualTo(environment.getVersion()); + fooProperty = ConfigServerTestUtils.getProperty(environment, "bar.properties", "foo"); assertThat("barNewCommit").isEqualTo(fooProperty); // request the prior commit ID and make sure we get it diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java index 060bb0fd85..d7be6ab04d 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java @@ -85,6 +85,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockingDetails; import static org.mockito.Mockito.never; @@ -800,17 +801,35 @@ public void testRefreshWithoutFetch() throws Exception { repo.setUri("http://somegitserver/somegitrepo"); repo.setBasedir(this.basedir); - // Set the refresh rate to 2 seconds and last update before 100ms. There should be - // no remote repo fetch. + // Set the refresh rate to 10 seconds and last update before 100ms. There should be + // no remote repo fetch and not merge. repo.setLastRefresh(System.currentTimeMillis() - 100); - repo.setRefreshRate(2); + repo.setRefreshRate(10); repo.refresh("master"); // Verify no fetch nor merge. verify(git, never()).fetch(); verify(git).checkout(); - verify(git).merge(); + verify(git, never()).merge(); + + // the checkout should not be called anymore since we are on the same branch + clearInvocations(git); + repo.refresh("master"); + + // Verify no fetch, checkout or merge + verify(git, never()).fetch(); + verify(git, never()).checkout(); + verify(git, never()).merge(); + + // checkout another branch, but still before the cache expires + clearInvocations(git); + repo.refresh("staging"); + + // Verify no fetch, a checkout and still no merge + verify(git, never()).fetch(); + verify(git).checkout(); + verify(git, never()).merge(); } @Test From 5d30e4b4e8357c5f1365f7944be8d6a0de70424e Mon Sep 17 00:00:00 2001 From: Henri Tremblay Date: Thu, 5 Jun 2025 23:33:15 -0400 Subject: [PATCH 7/7] Add myself as an author Signed-off-by: Henri Tremblay --- .../config/server/environment/JGitEnvironmentRepository.java | 3 ++- .../environment/JGitEnvironmentRepositoryIntegrationTests.java | 1 + .../server/environment/JGitEnvironmentRepositoryTests.java | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java index d2c0970c4b..552519ed1a 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java @@ -79,6 +79,7 @@ * @author Ryan Lynch * @author Gareth Clay * @author ChaoDong Xi + * @author Henri Tremblay */ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository implements EnvironmentRepository, SearchPathLocator, InitializingBean { @@ -156,7 +157,7 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository * * @implNote * The field is not volatile since we tend to lock around the {@link #refresh(String)} method which uses it. - * However, this method is public si maybe it's a bad bet. + * However, this method is public so maybe it's a bad bet. */ private String previousLabel = "$%^&"; // none existing branch diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java index f64e78e68e..a4c2f5f96a 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java @@ -71,6 +71,7 @@ * @author Roy Clarkson * @author Daniel Lavoie * @author Ryan Lynch + * @author Henri Tremblay */ public class JGitEnvironmentRepositoryIntegrationTests { diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java index d7be6ab04d..ffe10683f8 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java @@ -97,6 +97,7 @@ /** * @author Dave Syer * @author Gareth Clay + * @author Henri Tremblay */ public class JGitEnvironmentRepositoryTests {