You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "michael-o (via GitHub)" <gi...@apache.org> on 2023/04/04 18:36:39 UTC
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #272: [MRESOLVER-346] Too eager locking in resolver
michael-o commented on code in PR #272:
URL: https://github.com/apache/maven-resolver/pull/272#discussion_r1157625854
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultDeployer.java:
##########
@@ -197,7 +197,7 @@ public DeployResult deploy(RepositorySystemSession session, DeployRequest reques
e);
}
- try (SyncContext syncContext = syncContextFactory.newInstance(session, false)) {
+ try (SyncContext syncContext = syncContextFactory.newInstance(session, true)) {
Review Comment:
Checksums and stuff are generated on the fly and never persisted on disk, as far as I remember?!
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -256,207 +257,226 @@ public List<ArtifactResult> resolveArtifacts(
artifacts.add(request.getArtifact());
}
- syncContext.acquire(artifacts, null);
-
- return resolve(session, requests);
+ return resolve(shared, exclusive, artifacts, session, requests);
}
}
@SuppressWarnings("checkstyle:methodlength")
private List<ArtifactResult> resolve(
- RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)
+ SyncContext shared,
+ SyncContext exclusive,
+ Collection<Artifact> subjects,
+ RepositorySystemSession session,
+ Collection<? extends ArtifactRequest> requests)
throws ArtifactResolutionException {
- List<ArtifactResult> results = new ArrayList<>(requests.size());
- boolean failures = false;
- final boolean simpleLrmInterop = ConfigUtils.getBoolean(session, false, CONFIG_PROP_SIMPLE_LRM_INTEROP);
+ SyncContext current = shared;
+ try {
+ while (true) {
Review Comment:
Can we end up in an endless loop here? Whould a do while loop with a condition make more sense here? We need at least one iteration.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -256,207 +257,226 @@ public List<ArtifactResult> resolveArtifacts(
artifacts.add(request.getArtifact());
}
- syncContext.acquire(artifacts, null);
-
- return resolve(session, requests);
+ return resolve(shared, exclusive, artifacts, session, requests);
}
}
@SuppressWarnings("checkstyle:methodlength")
private List<ArtifactResult> resolve(
- RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)
+ SyncContext shared,
+ SyncContext exclusive,
+ Collection<Artifact> subjects,
+ RepositorySystemSession session,
+ Collection<? extends ArtifactRequest> requests)
throws ArtifactResolutionException {
- List<ArtifactResult> results = new ArrayList<>(requests.size());
- boolean failures = false;
- final boolean simpleLrmInterop = ConfigUtils.getBoolean(session, false, CONFIG_PROP_SIMPLE_LRM_INTEROP);
+ SyncContext current = shared;
+ try {
+ while (true) {
+ current.acquire(subjects, null);
- LocalRepositoryManager lrm = session.getLocalRepositoryManager();
- WorkspaceReader workspace = session.getWorkspaceReader();
+ boolean failures = false;
+ final List<ArtifactResult> results = new ArrayList<>(requests.size());
+ final boolean simpleLrmInterop = ConfigUtils.getBoolean(session, false, CONFIG_PROP_SIMPLE_LRM_INTEROP);
+ final LocalRepositoryManager lrm = session.getLocalRepositoryManager();
+ final WorkspaceReader workspace = session.getWorkspaceReader();
+ final List<ResolutionGroup> groups = new ArrayList<>();
+ // filter != null: means "filtering applied", if null no filtering applied (behave as before)
+ final RemoteRepositoryFilter filter = remoteRepositoryFilterManager.getRemoteRepositoryFilter(session);
- List<ResolutionGroup> groups = new ArrayList<>();
- // filter != null: means "filtering applied", if null no filtering applied (behave as before)
- RemoteRepositoryFilter filter = remoteRepositoryFilterManager.getRemoteRepositoryFilter(session);
+ for (ArtifactRequest request : requests) {
+ RequestTrace trace = RequestTrace.newChild(request.getTrace(), request);
- for (ArtifactRequest request : requests) {
- RequestTrace trace = RequestTrace.newChild(request.getTrace(), request);
+ ArtifactResult result = new ArtifactResult(request);
+ results.add(result);
- ArtifactResult result = new ArtifactResult(request);
- results.add(result);
+ Artifact artifact = request.getArtifact();
- Artifact artifact = request.getArtifact();
+ if (current == shared) {
+ artifactResolving(session, trace, artifact);
+ }
- artifactResolving(session, trace, artifact);
+ String localPath = artifact.getProperty(ArtifactProperties.LOCAL_PATH, null);
+ if (localPath != null) {
+ // unhosted artifact, just validate file
+ File file = new File(localPath);
+ if (!file.isFile()) {
+ failures = true;
+ result.addException(new ArtifactNotFoundException(artifact, null));
+ } else {
+ artifact = artifact.setFile(file);
+ result.setArtifact(artifact);
+ artifactResolved(session, trace, artifact, null, result.getExceptions());
+ }
+ continue;
+ }
- String localPath = artifact.getProperty(ArtifactProperties.LOCAL_PATH, null);
- if (localPath != null) {
- // unhosted artifact, just validate file
- File file = new File(localPath);
- if (!file.isFile()) {
- failures = true;
- result.addException(new ArtifactNotFoundException(artifact, null));
- } else {
- artifact = artifact.setFile(file);
- result.setArtifact(artifact);
- artifactResolved(session, trace, artifact, null, result.getExceptions());
- }
- continue;
- }
+ List<RemoteRepository> remoteRepositories = request.getRepositories();
+ List<RemoteRepository> filteredRemoteRepositories = new ArrayList<>(remoteRepositories);
+ if (filter != null) {
+ for (RemoteRepository repository : remoteRepositories) {
+ RemoteRepositoryFilter.Result filterResult = filter.acceptArtifact(repository, artifact);
+ if (!filterResult.isAccepted()) {
+ result.addException(new ArtifactFilteredOutException(
+ artifact, repository, filterResult.reasoning()));
+ filteredRemoteRepositories.remove(repository);
+ }
+ }
+ }
- List<RemoteRepository> remoteRepositories = request.getRepositories();
- List<RemoteRepository> filteredRemoteRepositories = new ArrayList<>(remoteRepositories);
- if (filter != null) {
- for (RemoteRepository repository : remoteRepositories) {
- RemoteRepositoryFilter.Result filterResult = filter.acceptArtifact(repository, artifact);
- if (!filterResult.isAccepted()) {
- result.addException(
- new ArtifactFilteredOutException(artifact, repository, filterResult.reasoning()));
- filteredRemoteRepositories.remove(repository);
+ VersionResult versionResult;
+ try {
+ VersionRequest versionRequest =
+ new VersionRequest(artifact, filteredRemoteRepositories, request.getRequestContext());
+ versionRequest.setTrace(trace);
+ versionResult = versionResolver.resolveVersion(session, versionRequest);
+ } catch (VersionResolutionException e) {
+ result.addException(e);
+ continue;
}
- }
- }
- VersionResult versionResult;
- try {
- VersionRequest versionRequest =
- new VersionRequest(artifact, filteredRemoteRepositories, request.getRequestContext());
- versionRequest.setTrace(trace);
- versionResult = versionResolver.resolveVersion(session, versionRequest);
- } catch (VersionResolutionException e) {
- result.addException(e);
- continue;
- }
+ artifact = artifact.setVersion(versionResult.getVersion());
- artifact = artifact.setVersion(versionResult.getVersion());
+ if (versionResult.getRepository() != null) {
+ if (versionResult.getRepository() instanceof RemoteRepository) {
+ filteredRemoteRepositories =
+ Collections.singletonList((RemoteRepository) versionResult.getRepository());
+ } else {
+ filteredRemoteRepositories = Collections.emptyList();
+ }
+ }
- if (versionResult.getRepository() != null) {
- if (versionResult.getRepository() instanceof RemoteRepository) {
- filteredRemoteRepositories =
- Collections.singletonList((RemoteRepository) versionResult.getRepository());
- } else {
- filteredRemoteRepositories = Collections.emptyList();
- }
- }
+ if (workspace != null) {
+ File file = workspace.findArtifact(artifact);
+ if (file != null) {
+ artifact = artifact.setFile(file);
+ result.setArtifact(artifact);
+ result.setRepository(workspace.getRepository());
+ artifactResolved(session, trace, artifact, result.getRepository(), null);
+ continue;
+ }
+ }
- if (workspace != null) {
- File file = workspace.findArtifact(artifact);
- if (file != null) {
- artifact = artifact.setFile(file);
- result.setArtifact(artifact);
- result.setRepository(workspace.getRepository());
- artifactResolved(session, trace, artifact, result.getRepository(), null);
- continue;
- }
- }
+ LocalArtifactResult local = lrm.find(
+ session,
+ new LocalArtifactRequest(
+ artifact, filteredRemoteRepositories, request.getRequestContext()));
+ result.setLocalArtifactResult(local);
+ boolean found = (filter != null && local.isAvailable()) || isLocallyInstalled(local, versionResult);
+ // with filtering it is availability that drives logic
+ // without filtering it is simply presence of file that drives the logic
+ // "interop" logic with simple LRM leads to RRF breakage: hence is ignored when filtering in effect
+ if (found) {
+ if (local.getRepository() != null) {
+ result.setRepository(local.getRepository());
+ } else {
+ result.setRepository(lrm.getRepository());
+ }
+
+ try {
+ artifact = artifact.setFile(getFile(session, artifact, local.getFile()));
+ result.setArtifact(artifact);
+ artifactResolved(session, trace, artifact, result.getRepository(), null);
+ } catch (ArtifactTransferException e) {
+ result.addException(e);
+ }
+ if (filter == null && simpleLrmInterop && !local.isAvailable()) {
+ /*
+ * NOTE: Interop with simple local repository: An artifact installed by a simple local repo
+ * manager will not show up in the repository tracking file of the enhanced local repository.
+ * If however the maven-metadata-local.xml tells us the artifact was installed locally, we
+ * sync the repository tracking file.
+ */
+ lrm.add(session, new LocalArtifactRegistration(artifact));
+ }
+
+ continue;
+ }
- LocalArtifactResult local = lrm.find(
- session,
- new LocalArtifactRequest(artifact, filteredRemoteRepositories, request.getRequestContext()));
- result.setLocalArtifactResult(local);
- boolean found = (filter != null && local.isAvailable()) || isLocallyInstalled(local, versionResult);
- // with filtering it is availability that drives logic
- // without filtering it is simply presence of file that drives the logic
- // "interop" logic with simple LRM leads to RRF breakage: hence is ignored when filtering in effect
- if (found) {
- if (local.getRepository() != null) {
- result.setRepository(local.getRepository());
- } else {
- result.setRepository(lrm.getRepository());
- }
+ if (local.getFile() != null) {
+ LOGGER.info(
+ "Artifact {} is present in the local repository, but cached from a remote repository ID that is unavailable in current build context, verifying that is downloadable from {}",
+ artifact,
+ remoteRepositories);
+ }
- try {
- artifact = artifact.setFile(getFile(session, artifact, local.getFile()));
- result.setArtifact(artifact);
- artifactResolved(session, trace, artifact, result.getRepository(), null);
- } catch (ArtifactTransferException e) {
- result.addException(e);
- }
- if (filter == null && simpleLrmInterop && !local.isAvailable()) {
- /*
- * NOTE: Interop with simple local repository: An artifact installed by a simple local repo
- * manager will not show up in the repository tracking file of the enhanced local repository.
- * If however the maven-metadata-local.xml tells us the artifact was installed locally, we
- * sync the repository tracking file.
- */
- lrm.add(session, new LocalArtifactRegistration(artifact));
+ LOGGER.debug("Resolving artifact {} from {}", artifact, remoteRepositories);
+ AtomicBoolean resolved = new AtomicBoolean(false);
+ Iterator<ResolutionGroup> groupIt = groups.iterator();
+ for (RemoteRepository repo : filteredRemoteRepositories) {
+ if (!repo.getPolicy(artifact.isSnapshot()).isEnabled()) {
+ continue;
+ }
+
+ try {
+ Utils.checkOffline(session, offlineController, repo);
+ } catch (RepositoryOfflineException e) {
+ Exception exception = new ArtifactNotFoundException(
+ artifact,
+ repo,
+ "Cannot access " + repo.getId() + " ("
+ + repo.getUrl() + ") in offline mode and the artifact " + artifact
+ + " has not been downloaded from it before.",
+ e);
+ result.addException(exception);
+ continue;
+ }
+
+ ResolutionGroup group = null;
+ while (groupIt.hasNext()) {
+ ResolutionGroup t = groupIt.next();
+ if (t.matches(repo)) {
+ group = t;
+ break;
+ }
+ }
+ if (group == null) {
+ group = new ResolutionGroup(repo);
+ groups.add(group);
+ groupIt = Collections.emptyIterator();
+ }
+ group.items.add(new ResolutionItem(trace, artifact, resolved, result, local, repo));
+ }
}
- continue;
- }
-
- if (local.getFile() != null) {
- LOGGER.info(
- "Artifact {} is present in the local repository, but cached from a remote repository ID that is unavailable in current build context, verifying that is downloadable from {}",
- artifact,
- remoteRepositories);
- }
-
- LOGGER.debug("Resolving artifact {} from {}", artifact, remoteRepositories);
- AtomicBoolean resolved = new AtomicBoolean(false);
- Iterator<ResolutionGroup> groupIt = groups.iterator();
- for (RemoteRepository repo : filteredRemoteRepositories) {
- if (!repo.getPolicy(artifact.isSnapshot()).isEnabled()) {
+ if (!groups.isEmpty() && current == shared) {
Review Comment:
The not empty implies that resolution is not complete?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NamedLockFactoryAdapter.java:
##########
@@ -48,6 +49,14 @@ public final class NamedLockFactoryAdapter {
public static final TimeUnit DEFAULT_TIME_UNIT = TimeUnit.SECONDS;
+ public static final String RETRIES_KEY = "aether.syncContext.named.retries";
Review Comment:
This change is for me is not related and shoud be a separate ticket and PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org