[
https://issues.apache.org/jira/browse/MRESOLVER-346?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17708675#comment-17708675
]
ASF GitHub Bot commented on MRESOLVER-346:
------------------------------------------
cstamas commented on code in PR #272:
URL: https://github.com/apache/maven-resolver/pull/272#discussion_r1158052421
##########
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:
Yes, groups are "prepared download batches" (per remote repo, 1 or more
artifact). By group being not empty, means that resolver could not fulfill
resolution from local repo, hence is about to download. Here, the "downside" is
that when this happens, and we switch from shared to exclusive lock, groups are
rebuilt from scratch, so kinda we throw away this only to rinse and repeat. But
this is required for correctness, as between giving up shared lock, and
successfully getting exclusive lock, state of local repo may change.
> 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)
