[
https://issues.apache.org/jira/browse/MRESOLVER-346?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17708535#comment-17708535
]
ASF GitHub Bot commented on MRESOLVER-346:
------------------------------------------
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.
> Too eager locking
> -----------------
>
> Key: MRESOLVER-346
> URL: https://issues.apache.org/jira/browse/MRESOLVER-346
> Project: Maven Resolver
> Issue Type: Bug
> Components: Resolver
> Reporter: Tamas Cservenak
> Assignee: Tamas Cservenak
> Priority: Major
> Fix For: 1.9.8
>
>
> The locking that is present in Resolver since 1.8.0 is too eager:
> * there are no shared locks used at all
> * this implies that local repository access is heavily serialized by locking
> * there is no "upgrade" of locking due that above
> * consequence is that "hot" artifacts in bigger multi module build run in
> parallel become bottleneck as all threads will wait for their moment to grab
> exclusive lock.
> * another consequence: our "wait time" (def 30s) becomes problem, as due that
> above, if build grows, the plausible "wait time" (as all lock is exclusive,
> but requester count grows) grows as well. Also, this means we have threads
> there doing nothing, just sitting as they wait for exclusive lock one after
> another.
> We can do it better: there are 4 main areas where locking is used:
> * ArtifactInstaller: it is about to install (write) artifact files to local
> repository, it needs exclusive lock, *no change needed*.
> * ArtifactDeployer: it is about to upload present files to remote, it does
> not modifies anything on local. *Change its lock to shared*. The exclusive
> lock also meant that if no DeployAtEnd used, other modules during resolution
> had to wait while this module uploaded.
> * ArtifactResolver and MetadataResolver: two very similar beasts, they
> attempt to resolve locally (from local repo) w/o any content modification
> (read only), and if not successful, they will reach remote to download what
> is needed (write). Here we *could do something similar to
> [DCL|https://en.wikipedia.org/wiki/Double-checked_locking] is*: try with
> shared lock first, and if local content is not fulfilling, release shared,
> acquire exclusive and REDO all (as meanwhile someone else may downloaded
> files already).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
