From 393b2a73905e0c741a67872cd9979e125ce1a685 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Tue, 11 Oct 2022 12:30:54 +0200 Subject: [PATCH 1/5] [MRESOLVER-278] Session close and onClose hooks Make session have close() method, denoting that given session instance is "done". This is integrator app obligation ot invoke this method. Also introdue onClose callbacks, that various components may use to hook when session is marked "done" to perform various cleanups. --- https://issues.apache.org/jira/browse/MRESOLVER-278 --- ...ractForwardingRepositorySystemSession.java | 24 ++++ .../DefaultRepositorySystemSession.java | 114 +++++++++++++----- .../eclipse/aether/MultiRuntimeException.java | 74 ++++++++++++ .../aether/RepositorySystemSession.java | 48 ++++++++ .../DefaultRepositorySystemSessionTest.java | 5 +- .../sisu/SisuRepositorySystemDemoModule.java | 4 +- maven-resolver-impl/pom.xml | 5 - .../impl/DefaultRepositorySystem.java | 4 + .../SummaryFileTrustedChecksumsSource.java | 73 ++++++++--- .../DefaultSyncContextFactory.java | 71 ++++------- 10 files changed, 324 insertions(+), 98 deletions(-) create mode 100644 maven-resolver-api/src/main/java/org/eclipse/aether/MultiRuntimeException.java diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/AbstractForwardingRepositorySystemSession.java b/maven-resolver-api/src/main/java/org/eclipse/aether/AbstractForwardingRepositorySystemSession.java index 008a2df55..8fe24c1ab 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/AbstractForwardingRepositorySystemSession.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/AbstractForwardingRepositorySystemSession.java @@ -20,6 +20,7 @@ */ import java.util.Map; +import java.util.function.Consumer; import org.eclipse.aether.artifact.ArtifactTypeRegistry; import org.eclipse.aether.collection.DependencyGraphTransformer; @@ -218,4 +219,27 @@ public FileTransformerManager getFileTransformerManager() return getSession().getFileTransformerManager(); } + @Override + public final void addOnCloseHandler( Consumer handler ) + { + getSession().addOnCloseHandler( handler ); + } + + @Override + public final boolean isClosed() + { + return getSession().isClosed(); + } + + /** + * This method is special: by default it throws (nested session should never be closed), the "top level" session + * should be closed instead. Still, this method is NOT {@code final}, to allow implementations overriding it, + * and in case when needed, handle forwarded session as "top level" session. + */ + @Override + public void close() + { + throw new IllegalStateException( "Forwarding session should not be closed, " + + "close the top-level (forwarded) session instead." ); + } } diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java b/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java index 90b32f48f..7a8bba5f6 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java @@ -19,12 +19,17 @@ * under the License. */ +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.ListIterator; import java.util.Map; import static java.util.Objects.requireNonNull; import java.util.Collection; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; import org.eclipse.aether.artifact.Artifact; import org.eclipse.aether.artifact.ArtifactType; @@ -62,6 +67,8 @@ public final class DefaultRepositorySystemSession implements RepositorySystemSession { + private final AtomicBoolean closed; + private boolean readOnly; private boolean offline; @@ -127,6 +134,7 @@ public final class DefaultRepositorySystemSession */ public DefaultRepositorySystemSession() { + closed = new AtomicBoolean( false ); systemProperties = new HashMap<>(); systemPropertiesView = Collections.unmodifiableMap( systemProperties ); userProperties = new HashMap<>(); @@ -153,6 +161,7 @@ public DefaultRepositorySystemSession( RepositorySystemSession session ) { requireNonNull( session, "repository system session cannot be null" ); + closed = new AtomicBoolean( false ); setOffline( session.isOffline() ); setIgnoreArtifactDescriptorRepositories( session.isIgnoreArtifactDescriptorRepositories() ); setResolutionErrorPolicy( session.getResolutionErrorPolicy() ); @@ -194,7 +203,7 @@ public boolean isOffline() */ public DefaultRepositorySystemSession setOffline( boolean offline ) { - failIfReadOnly(); + verifyStateForMutation(); this.offline = offline; return this; } @@ -215,7 +224,7 @@ public boolean isIgnoreArtifactDescriptorRepositories() public DefaultRepositorySystemSession setIgnoreArtifactDescriptorRepositories( boolean ignoreArtifactDescriptorRepositories ) { - failIfReadOnly(); + verifyStateForMutation(); this.ignoreArtifactDescriptorRepositories = ignoreArtifactDescriptorRepositories; return this; } @@ -234,7 +243,7 @@ public ResolutionErrorPolicy getResolutionErrorPolicy() */ public DefaultRepositorySystemSession setResolutionErrorPolicy( ResolutionErrorPolicy resolutionErrorPolicy ) { - failIfReadOnly(); + verifyStateForMutation(); this.resolutionErrorPolicy = resolutionErrorPolicy; return this; } @@ -254,7 +263,7 @@ public ArtifactDescriptorPolicy getArtifactDescriptorPolicy() public DefaultRepositorySystemSession setArtifactDescriptorPolicy( ArtifactDescriptorPolicy artifactDescriptorPolicy ) { - failIfReadOnly(); + verifyStateForMutation(); this.artifactDescriptorPolicy = artifactDescriptorPolicy; return this; } @@ -276,7 +285,7 @@ public String getChecksumPolicy() */ public DefaultRepositorySystemSession setChecksumPolicy( String checksumPolicy ) { - failIfReadOnly(); + verifyStateForMutation(); this.checksumPolicy = checksumPolicy; return this; } @@ -298,7 +307,7 @@ public String getUpdatePolicy() */ public DefaultRepositorySystemSession setUpdatePolicy( String updatePolicy ) { - failIfReadOnly(); + verifyStateForMutation(); this.updatePolicy = updatePolicy; return this; } @@ -323,7 +332,7 @@ public LocalRepositoryManager getLocalRepositoryManager() */ public DefaultRepositorySystemSession setLocalRepositoryManager( LocalRepositoryManager localRepositoryManager ) { - failIfReadOnly(); + verifyStateForMutation(); this.localRepositoryManager = localRepositoryManager; return this; } @@ -336,7 +345,7 @@ public FileTransformerManager getFileTransformerManager() public DefaultRepositorySystemSession setFileTransformerManager( FileTransformerManager fileTransformerManager ) { - failIfReadOnly(); + verifyStateForMutation(); this.fileTransformerManager = fileTransformerManager; if ( this.fileTransformerManager == null ) { @@ -359,7 +368,7 @@ public WorkspaceReader getWorkspaceReader() */ public DefaultRepositorySystemSession setWorkspaceReader( WorkspaceReader workspaceReader ) { - failIfReadOnly(); + verifyStateForMutation(); this.workspaceReader = workspaceReader; return this; } @@ -377,7 +386,7 @@ public RepositoryListener getRepositoryListener() */ public DefaultRepositorySystemSession setRepositoryListener( RepositoryListener repositoryListener ) { - failIfReadOnly(); + verifyStateForMutation(); this.repositoryListener = repositoryListener; return this; } @@ -395,7 +404,7 @@ public TransferListener getTransferListener() */ public DefaultRepositorySystemSession setTransferListener( TransferListener transferListener ) { - failIfReadOnly(); + verifyStateForMutation(); this.transferListener = transferListener; return this; } @@ -444,7 +453,7 @@ public Map getSystemProperties() */ public DefaultRepositorySystemSession setSystemProperties( Map systemProperties ) { - failIfReadOnly(); + verifyStateForMutation(); this.systemProperties = copySafe( systemProperties, String.class ); systemPropertiesView = Collections.unmodifiableMap( this.systemProperties ); return this; @@ -459,7 +468,7 @@ public DefaultRepositorySystemSession setSystemProperties( Map systemPrope */ public DefaultRepositorySystemSession setSystemProperty( String key, String value ) { - failIfReadOnly(); + verifyStateForMutation(); if ( value != null ) { systemProperties.put( key, value ); @@ -489,7 +498,7 @@ public Map getUserProperties() */ public DefaultRepositorySystemSession setUserProperties( Map userProperties ) { - failIfReadOnly(); + verifyStateForMutation(); this.userProperties = copySafe( userProperties, String.class ); userPropertiesView = Collections.unmodifiableMap( this.userProperties ); return this; @@ -504,7 +513,7 @@ public DefaultRepositorySystemSession setUserProperties( Map userPropertie */ public DefaultRepositorySystemSession setUserProperty( String key, String value ) { - failIfReadOnly(); + verifyStateForMutation(); if ( value != null ) { userProperties.put( key, value ); @@ -533,7 +542,7 @@ public Map getConfigProperties() */ public DefaultRepositorySystemSession setConfigProperties( Map configProperties ) { - failIfReadOnly(); + verifyStateForMutation(); this.configProperties = copySafe( configProperties, Object.class ); configPropertiesView = Collections.unmodifiableMap( this.configProperties ); return this; @@ -548,7 +557,7 @@ public DefaultRepositorySystemSession setConfigProperties( Map configPrope */ public DefaultRepositorySystemSession setConfigProperty( String key, Object value ) { - failIfReadOnly(); + verifyStateForMutation(); if ( value != null ) { configProperties.put( key, value ); @@ -575,7 +584,7 @@ public MirrorSelector getMirrorSelector() */ public DefaultRepositorySystemSession setMirrorSelector( MirrorSelector mirrorSelector ) { - failIfReadOnly(); + verifyStateForMutation(); this.mirrorSelector = mirrorSelector; if ( this.mirrorSelector == null ) { @@ -600,7 +609,7 @@ public ProxySelector getProxySelector() */ public DefaultRepositorySystemSession setProxySelector( ProxySelector proxySelector ) { - failIfReadOnly(); + verifyStateForMutation(); this.proxySelector = proxySelector; if ( this.proxySelector == null ) { @@ -625,7 +634,7 @@ public AuthenticationSelector getAuthenticationSelector() */ public DefaultRepositorySystemSession setAuthenticationSelector( AuthenticationSelector authenticationSelector ) { - failIfReadOnly(); + verifyStateForMutation(); this.authenticationSelector = authenticationSelector; if ( this.authenticationSelector == null ) { @@ -647,7 +656,7 @@ public ArtifactTypeRegistry getArtifactTypeRegistry() */ public DefaultRepositorySystemSession setArtifactTypeRegistry( ArtifactTypeRegistry artifactTypeRegistry ) { - failIfReadOnly(); + verifyStateForMutation(); this.artifactTypeRegistry = artifactTypeRegistry; if ( this.artifactTypeRegistry == null ) { @@ -669,7 +678,7 @@ public DependencyTraverser getDependencyTraverser() */ public DefaultRepositorySystemSession setDependencyTraverser( DependencyTraverser dependencyTraverser ) { - failIfReadOnly(); + verifyStateForMutation(); this.dependencyTraverser = dependencyTraverser; return this; } @@ -687,7 +696,7 @@ public DependencyManager getDependencyManager() */ public DefaultRepositorySystemSession setDependencyManager( DependencyManager dependencyManager ) { - failIfReadOnly(); + verifyStateForMutation(); this.dependencyManager = dependencyManager; return this; } @@ -705,7 +714,7 @@ public DependencySelector getDependencySelector() */ public DefaultRepositorySystemSession setDependencySelector( DependencySelector dependencySelector ) { - failIfReadOnly(); + verifyStateForMutation(); this.dependencySelector = dependencySelector; return this; } @@ -724,7 +733,7 @@ public VersionFilter getVersionFilter() */ public DefaultRepositorySystemSession setVersionFilter( VersionFilter versionFilter ) { - failIfReadOnly(); + verifyStateForMutation(); this.versionFilter = versionFilter; return this; } @@ -744,7 +753,7 @@ public DependencyGraphTransformer getDependencyGraphTransformer() public DefaultRepositorySystemSession setDependencyGraphTransformer( DependencyGraphTransformer dependencyGraphTransformer ) { - failIfReadOnly(); + verifyStateForMutation(); this.dependencyGraphTransformer = dependencyGraphTransformer; return this; } @@ -762,7 +771,7 @@ public SessionData getData() */ public DefaultRepositorySystemSession setData( SessionData data ) { - failIfReadOnly(); + verifyStateForMutation(); this.data = data; if ( this.data == null ) { @@ -784,7 +793,7 @@ public RepositoryCache getCache() */ public DefaultRepositorySystemSession setCache( RepositoryCache cache ) { - failIfReadOnly(); + verifyStateForMutation(); this.cache = cache; return this; } @@ -799,12 +808,19 @@ public void setReadOnly() readOnly = true; } - private void failIfReadOnly() + /** + * Verifies this instance state for mutation operations: mutated instance must not be read-only or closed. + */ + private void verifyStateForMutation() { if ( readOnly ) { throw new IllegalStateException( "repository system session is read-only" ); } + if ( isClosed() ) + { + throw new IllegalStateException( "repository system session is closed" ); + } } static class NullProxySelector @@ -873,4 +889,44 @@ public Collection getTransformersForArtifact( Artifact artifact } } + private final CopyOnWriteArrayList> onCloseHandlers + = new CopyOnWriteArrayList<>(); + + @Override + public void addOnCloseHandler( Consumer handler ) + { + verifyStateForMutation(); + requireNonNull( handler, "handler cannot be null" ); + onCloseHandlers.add( handler ); + } + + @Override + public boolean isClosed() + { + return closed.get(); + } + + @Override + public void close() + { + if ( closed.compareAndSet( false, true ) ) + { + ArrayList exceptions = new ArrayList<>(); + ListIterator> handlerIterator + = onCloseHandlers.listIterator( onCloseHandlers.size() ); + while ( handlerIterator.hasPrevious() ) + { + Consumer handler = handlerIterator.previous(); + try + { + handler.accept( this ); + } + catch ( Exception e ) + { + exceptions.add( e ); + } + } + MultiRuntimeException.mayThrow( "session onClose handler failures", exceptions ); + } + } } diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/MultiRuntimeException.java b/maven-resolver-api/src/main/java/org/eclipse/aether/MultiRuntimeException.java new file mode 100644 index 000000000..c71484192 --- /dev/null +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/MultiRuntimeException.java @@ -0,0 +1,74 @@ +package org.eclipse.aether; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import java.util.List; + +import static java.util.Objects.requireNonNull; + +/** + * Runtime exception to be thrown when multiple actions were executed and one or more failed. To be used when no + * fallback on resolver side is needed or is possible. + * + * @since TBD + */ +public final class MultiRuntimeException + extends RuntimeException +{ + private final List throwable; + + private MultiRuntimeException( String message, List throwable ) + { + super( message ); + this.throwable = throwable; + for ( Throwable t : throwable ) + { + addSuppressed( t ); + } + } + + /** + * Returns the list of throwable that is wrapped in this exception. + * + * @return The list of throwable, never {@code null}. + */ + public List getThrowable() + { + return throwable; + } + + /** + * Helper method that receives a (non-null) message and (non-null) list of throwable, and following happens: + *
    + *
  • if list is empty - nothing
    • + *
  • if list not empty - {@link MultiRuntimeException} is thrown wrapping all elements
    • + *
+ */ + public static void mayThrow( String message, List throwable ) + { + requireNonNull( message ); + requireNonNull( throwable ); + + if ( !throwable.isEmpty() ) + { + throw new MultiRuntimeException( message, throwable ); + } + } +} diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java b/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java index 7aef49af2..68b504e31 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java @@ -20,6 +20,7 @@ */ import java.util.Map; +import java.util.function.Consumer; import org.eclipse.aether.artifact.ArtifactTypeRegistry; import org.eclipse.aether.collection.DependencyGraphTransformer; @@ -271,4 +272,51 @@ public interface RepositorySystemSession @Deprecated FileTransformerManager getFileTransformerManager(); + /** + * Adds "on close" handler to this session, it must not be {@code null}. Note: when handlers are invoked, the + * passed in (this) session is ALREADY CLOSED (the {@link #isClosed()} method returns {@code true}). This implies, + * that handlers cannot use {@link RepositorySystem} to resolve/collect/and so on, handlers are meant to perform + * some internal cleanup on session close. Attempt to add handler to closed session will throw. + * + * @since TBD + */ + void addOnCloseHandler( Consumer handler ); + + /** + * Returns {@code true} if this instance was already closed. Closed sessions should NOT be used anymore. + * + * @return {@code true} if session was closed. + * @since TBD + */ + boolean isClosed(); + + /** + * Closes this session and possibly releases related resources by invoking "on close" handlers added by + * {@link #addOnCloseHandler(Consumer)} method. This method may be invoked multiple times, + * but only first invocation will actually invoke handlers, subsequent invocations will be no-op. + *

+ * When close action happens, all the registered handlers will be invoked (even if some throws), and at end + * of operation a {@link MultiRuntimeException} may be thrown signaling that some handler(s) failed. This exception + * may be ignored, is at the discretion of caller. + *

+ * Important: ideally it is the session creator who should be responsible to close the session. While "nested" + * (wrapped) sessions {@link AbstractForwardingRepositorySystemSession} and alike) are able to close session, + * they should not do that. The pattern that is recommended is like: + * + * 

 {@code
+     * RepositorySystemSession session = create session...
+     * try
+     * {
+     *   ... use/nest session
+     * }
+     * finally
+     * {
+     *   session.close();
+     * }
+     * }
+ * + * @since TBD + */ + void close(); + } diff --git a/maven-resolver-api/src/test/java/org/eclipse/aether/DefaultRepositorySystemSessionTest.java b/maven-resolver-api/src/test/java/org/eclipse/aether/DefaultRepositorySystemSessionTest.java index ccca26ad6..1fe084640 100644 --- a/maven-resolver-api/src/test/java/org/eclipse/aether/DefaultRepositorySystemSessionTest.java +++ b/maven-resolver-api/src/test/java/org/eclipse/aether/DefaultRepositorySystemSessionTest.java @@ -140,7 +140,10 @@ public void testCopyRepositorySystemSession() throws Exception for ( Method method : methods ) { - assertEquals( method.getName(), method.invoke( session ) == null, method.invoke( newSession ) == null ); + if ( method.getParameterCount() == 0 ) + { + assertEquals( method.getName(), method.invoke( session ) == null, method.invoke( newSession ) == null ); + } } } diff --git a/maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java b/maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java index 71adb55b7..17a38061e 100644 --- a/maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java +++ b/maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java @@ -23,7 +23,6 @@ import com.google.inject.Binder; import com.google.inject.Module; -import org.eclipse.sisu.bean.LifecycleModule; import org.eclipse.sisu.inject.MutableBeanLocator; import org.eclipse.sisu.wire.ParameterKeys; @@ -35,7 +34,8 @@ public class SisuRepositorySystemDemoModule implements Module @Override public void configure( final Binder binder ) { - binder.install( new LifecycleModule() ); + // Resolver does not use anymore PostConstruct/PreDestroy annotations + // binder.install( new LifecycleModule() ); // NOTE: Maven 3.8.1 used in demo has Sisu Index for ALL components (older Maven does NOT have!) binder.bind( ParameterKeys.PROPERTIES ).toInstance( System.getProperties() ); binder.bind( ShutdownThread.class ).asEagerSingleton(); diff --git a/maven-resolver-impl/pom.xml b/maven-resolver-impl/pom.xml index 70d0bc668..0f9fe22f8 100644 --- a/maven-resolver-impl/pom.xml +++ b/maven-resolver-impl/pom.xml @@ -68,11 +68,6 @@ provided true - - javax.annotation - javax.annotation-api - 1.3.2 - org.eclipse.sisu org.eclipse.sisu.inject diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java index f079cff6a..1f7a1c2e6 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java @@ -466,6 +466,10 @@ private void validateSession( RepositorySystemSession session ) invalidSession( session.getAuthenticationSelector(), "authentication selector" ); invalidSession( session.getArtifactTypeRegistry(), "artifact type registry" ); invalidSession( session.getData(), "data" ); + if ( session.isClosed() ) + { + throw new IllegalStateException( "session is already closed" ); + } } private void validateRepositories( List repositories ) diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java index d6f7e21cc..4568723b1 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java @@ -29,13 +29,17 @@ import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; -import java.nio.file.StandardOpenOption; +import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; +import org.eclipse.aether.MultiRuntimeException; import org.eclipse.aether.RepositorySystemSession; import org.eclipse.aether.artifact.Artifact; import org.eclipse.aether.repository.ArtifactRepository; @@ -74,6 +78,8 @@ public final class SummaryFileTrustedChecksumsSource private static final String CHECKSUMS_CACHE_KEY = SummaryFileTrustedChecksumsSource.class.getName() + ".checksums"; + private static final String RECORDING_KEY = SummaryFileTrustedChecksumsSource.class.getName() + ".recording"; + private static final Logger LOGGER = LoggerFactory.getLogger( SummaryFileTrustedChecksumsSource.class ); public SummaryFileTrustedChecksumsSource() @@ -111,10 +117,17 @@ protected Map performLookup( RepositorySystemSession session, return checksums; } + @SuppressWarnings( "unchecked" ) @Override protected SummaryFileWriter getWriter( RepositorySystemSession session, Path basedir ) { - return new SummaryFileWriter( basedir, isOriginAware( session ) ); + ConcurrentHashMap> recordedLines = (ConcurrentHashMap>) session.getData() + .computeIfAbsent( RECORDING_KEY, () -> + { + session.addOnCloseHandler( this::saveSessionRecordedLines ); + return new ConcurrentHashMap<>(); + } ); + return new SummaryFileWriter( basedir, isOriginAware( session ), recordedLines ); } private ConcurrentHashMap loadProvidedChecksums( Path basedir, @@ -201,41 +214,38 @@ private String calculateSummaryPath( boolean originAware, return fileName + "." + checksumAlgorithmFactory.getFileExtension(); } - /** - * Note: this implementation will work only in single-thread (T1) model. While not ideal, the "workaround" is - * possible in both, Maven and Maven Daemon: force single threaded execution model while "recording" (in mvn: - * do not pass any {@code -T} CLI parameter, while for mvnd use {@code -1} CLI parameter. - * - * TODO: this will need to be reworked for at least two reasons: a) avoid duplicates in summary file and b) - * support multi threaded builds (probably will need "on session close" hook). - */ private class SummaryFileWriter implements Writer { private final Path basedir; private final boolean originAware; - private SummaryFileWriter( Path basedir, boolean originAware ) + private final ConcurrentHashMap> recordedLines; + + private SummaryFileWriter( Path basedir, + boolean originAware, + ConcurrentHashMap> recordedLines ) { this.basedir = basedir; this.originAware = originAware; + this.recordedLines = recordedLines; } @Override public void addTrustedArtifactChecksums( Artifact artifact, ArtifactRepository artifactRepository, List checksumAlgorithmFactories, - Map trustedArtifactChecksums ) throws IOException + Map trustedArtifactChecksums ) { for ( ChecksumAlgorithmFactory checksumAlgorithmFactory : checksumAlgorithmFactories ) { String checksum = requireNonNull( trustedArtifactChecksums.get( checksumAlgorithmFactory.getName() ) ); - String summaryLine = ArtifactIdUtils.toId( artifact ) + " " + checksum + "\n"; + String summaryLine = ArtifactIdUtils.toId( artifact ) + " " + checksum; Path summaryPath = basedir.resolve( calculateSummaryPath( originAware, artifactRepository, checksumAlgorithmFactory ) ); - Files.createDirectories( summaryPath.getParent() ); - Files.write( summaryPath, summaryLine.getBytes( StandardCharsets.UTF_8 ), - StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.APPEND ); + + recordedLines.computeIfAbsent( summaryPath, p -> Collections.synchronizedSet( new TreeSet<>() ) ) + .add( summaryLine ); } } @@ -245,4 +255,35 @@ public void close() // nop } } + + @SuppressWarnings( "unchecked" ) + private void saveSessionRecordedLines( RepositorySystemSession session ) + { + Map> recordedLines = (Map>) session.getData().get( RECORDING_KEY ); + if ( recordedLines == null || recordedLines.isEmpty() ) + { + return; + } + + ArrayList exceptions = new ArrayList<>(); + for ( Map.Entry> entry : recordedLines.entrySet() ) + { + Path file = entry.getKey(); + Set lines = entry.getValue(); + if ( !lines.isEmpty() ) + { + LOGGER.info( "Saving {} checksums to '{}'", lines.size(), file ); + try + { + Files.createDirectories( file.getParent() ); + Files.write( file, lines ); + } + catch ( IOException e ) + { + exceptions.add( e ); + } + } + } + MultiRuntimeException.mayThrow( "session save checksums failure", exceptions ); + } } diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/DefaultSyncContextFactory.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/DefaultSyncContextFactory.java index 65ba3d736..d7f6bd6c7 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/DefaultSyncContextFactory.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/DefaultSyncContextFactory.java @@ -19,14 +19,12 @@ * under the License. */ -import javax.annotation.PreDestroy; import javax.inject.Inject; import javax.inject.Named; import javax.inject.Singleton; import java.util.HashMap; import java.util.Map; -import java.util.concurrent.CopyOnWriteArrayList; import org.eclipse.aether.RepositorySystemSession; import org.eclipse.aether.SyncContext; @@ -46,8 +44,6 @@ import org.eclipse.aether.spi.locator.ServiceLocator; import org.eclipse.aether.spi.synccontext.SyncContextFactory; import org.eclipse.aether.util.ConfigUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import static java.util.Objects.requireNonNull; @@ -59,8 +55,6 @@ public final class DefaultSyncContextFactory implements SyncContextFactory, Service { - private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContextFactory.class ); - private static final String ADAPTER_KEY = DefaultSyncContextFactory.class.getName() + ".adapter"; private static final String NAME_MAPPER_KEY = "aether.syncContext.named.nameMapper"; @@ -75,8 +69,6 @@ public final class DefaultSyncContextFactory private Map namedLockFactories; - private final CopyOnWriteArrayList createdAdapters = new CopyOnWriteArrayList<>(); - /** * Constructor used with DI, where factories are injected and selected based on key. */ @@ -122,50 +114,39 @@ public void initService( final ServiceLocator locator ) public SyncContext newInstance( final RepositorySystemSession session, final boolean shared ) { requireNonNull( session, "session cannot be null" ); - NamedLockFactoryAdapter adapter = - (NamedLockFactoryAdapter) session.getData().computeIfAbsent( - ADAPTER_KEY, - () -> createAdapter( session ) - ); + NamedLockFactoryAdapter adapter = getOrCreateSessionAdapter( session ); return adapter.newInstance( session, shared ); } - private NamedLockFactoryAdapter createAdapter( final RepositorySystemSession session ) + private NamedLockFactoryAdapter getOrCreateSessionAdapter( final RepositorySystemSession session ) { - String nameMapperName = ConfigUtils.getString( session, DEFAULT_NAME_MAPPER_NAME, NAME_MAPPER_KEY ); - String namedLockFactoryName = ConfigUtils.getString( session, DEFAULT_FACTORY_NAME, FACTORY_KEY ); - NameMapper nameMapper = nameMappers.get( nameMapperName ); - if ( nameMapper == null ) - { - throw new IllegalArgumentException( "Unknown NameMapper name: " + nameMapperName - + ", known ones: " + nameMappers.keySet() ); - } - NamedLockFactory namedLockFactory = namedLockFactories.get( namedLockFactoryName ); - if ( namedLockFactory == null ) + return (NamedLockFactoryAdapter) session.getData().computeIfAbsent( ADAPTER_KEY, () -> { - throw new IllegalArgumentException( "Unknown NamedLockFactory name: " + namedLockFactoryName - + ", known ones: " + namedLockFactories.keySet() ); - } - NamedLockFactoryAdapter adapter = new NamedLockFactoryAdapter( nameMapper, namedLockFactory ); - createdAdapters.add( adapter ); - return adapter; + String nameMapperName = ConfigUtils.getString( session, DEFAULT_NAME_MAPPER_NAME, NAME_MAPPER_KEY ); + String namedLockFactoryName = ConfigUtils.getString( session, DEFAULT_FACTORY_NAME, FACTORY_KEY ); + NameMapper nameMapper = nameMappers.get( nameMapperName ); + if ( nameMapper == null ) + { + throw new IllegalArgumentException( "Unknown NameMapper name: " + nameMapperName + + ", known ones: " + nameMappers.keySet() ); + } + NamedLockFactory namedLockFactory = namedLockFactories.get( namedLockFactoryName ); + if ( namedLockFactory == null ) + { + throw new IllegalArgumentException( "Unknown NamedLockFactory name: " + namedLockFactoryName + + ", known ones: " + namedLockFactories.keySet() ); + } + session.addOnCloseHandler( this::shutDownSessionAdapter ); + return new NamedLockFactoryAdapter( nameMapper, namedLockFactory ); + } ); } - @PreDestroy - public void shutdown() + private void shutDownSessionAdapter( RepositorySystemSession session ) { - LOGGER.debug( "Shutting down created adapters..." ); - createdAdapters.forEach( adapter -> - { - try - { - adapter.shutdown(); - } - catch ( Exception e ) - { - LOGGER.warn( "Could not shutdown: {}", adapter, e ); - } - } - ); + NamedLockFactoryAdapter adapter = (NamedLockFactoryAdapter) session.getData().get( ADAPTER_KEY ); + if ( adapter != null ) + { + adapter.shutdown(); + } } } From 0f44e43fb2bb73d305efe7b59d9baf93e561df16 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 13 Oct 2022 09:36:15 +0200 Subject: [PATCH 2/5] PR comments --- .../DefaultRepositorySystemSession.java | 10 +++---- .../eclipse/aether/MultiRuntimeException.java | 26 +++++++++---------- .../aether/RepositorySystemSession.java | 20 +++++++------- .../sisu/SisuRepositorySystemDemoModule.java | 2 -- 4 files changed, 26 insertions(+), 32 deletions(-) diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java b/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java index 7a8bba5f6..9e105c633 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java @@ -22,7 +22,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.ListIterator; import java.util.Map; import static java.util.Objects.requireNonNull; @@ -897,7 +896,7 @@ public void addOnCloseHandler( Consumer handler ) { verifyStateForMutation(); requireNonNull( handler, "handler cannot be null" ); - onCloseHandlers.add( handler ); + onCloseHandlers.add( 0, handler ); } @Override @@ -912,14 +911,11 @@ public void close() if ( closed.compareAndSet( false, true ) ) { ArrayList exceptions = new ArrayList<>(); - ListIterator> handlerIterator - = onCloseHandlers.listIterator( onCloseHandlers.size() ); - while ( handlerIterator.hasPrevious() ) + for ( Consumer onCloseHandler : onCloseHandlers ) { - Consumer handler = handlerIterator.previous(); try { - handler.accept( this ); + onCloseHandler.accept( this ); } catch ( Exception e ) { diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/MultiRuntimeException.java b/maven-resolver-api/src/main/java/org/eclipse/aether/MultiRuntimeException.java index c71484192..24d48f935 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/MultiRuntimeException.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/MultiRuntimeException.java @@ -32,26 +32,26 @@ public final class MultiRuntimeException extends RuntimeException { - private final List throwable; + private final List throwables; - private MultiRuntimeException( String message, List throwable ) + private MultiRuntimeException( String message, List throwables ) { super( message ); - this.throwable = throwable; - for ( Throwable t : throwable ) + this.throwables = throwables; + for ( Throwable throwable : throwables ) { - addSuppressed( t ); + addSuppressed( throwable ); } } /** - * Returns the list of throwable that is wrapped in this exception. + * Returns the list of throwables that are wrapped in this exception. * - * @return The list of throwable, never {@code null}. + * @return The list of throwables, never {@code null}. */ - public List getThrowable() + public List getThrowables() { - return throwable; + return throwables; } /** @@ -61,14 +61,14 @@ public List getThrowable() *
  • if list not empty - {@link MultiRuntimeException} is thrown wrapping all elements
    • * */ - public static void mayThrow( String message, List throwable ) + public static void mayThrow( String message, List throwables ) { requireNonNull( message ); - requireNonNull( throwable ); + requireNonNull( throwables ); - if ( !throwable.isEmpty() ) + if ( !throwables.isEmpty() ) { - throw new MultiRuntimeException( message, throwable ); + throw new MultiRuntimeException( message, throwables ); } } } diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java b/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java index 68b504e31..1ce3a27df 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java @@ -49,7 +49,7 @@ * @noimplement This interface is not intended to be implemented by clients. * @noextend This interface is not intended to be extended by clients. */ -public interface RepositorySystemSession +public interface RepositorySystemSession extends AutoCloseable { /** @@ -299,24 +299,24 @@ public interface RepositorySystemSession * of operation a {@link MultiRuntimeException} may be thrown signaling that some handler(s) failed. This exception * may be ignored, is at the discretion of caller. *

    - * Important: ideally it is the session creator who should be responsible to close the session. While "nested" - * (wrapped) sessions {@link AbstractForwardingRepositorySystemSession} and alike) are able to close session, - * they should not do that. The pattern that is recommended is like: + * Important: ideally it is the session creator who should be responsible to close the session. The "nested" + * (delegating, wrapped) sessions {@link AbstractForwardingRepositorySystemSession} and alike) by default + * (without overriding the {@link AbstractForwardingRepositorySystemSession#close()} method) are not able to close + * session, and it is the "recommended" behaviour as well. The session "copy" (using copy-constructor) creates + * a copy instance, that does NOT share "closed" (nor "read-only") state with original session. + *

    + * The recommended pattern for "top level" sessions is the usual try-with-resource: * * 

     {@code
-     * RepositorySystemSession session = create session...
-     * try
+     * try ( RepositorySystemSession session = create session... )
      * {
      *   ... use/nest session
      * }
-     * finally
-     * {
-     *   session.close();
-     * }
      * }
    * * @since TBD */ + @Override void close(); } diff --git a/maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java b/maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java index 17a38061e..d4a43c8fd 100644 --- a/maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java +++ b/maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java @@ -34,8 +34,6 @@ public class SisuRepositorySystemDemoModule implements Module @Override public void configure( final Binder binder ) { - // Resolver does not use anymore PostConstruct/PreDestroy annotations - // binder.install( new LifecycleModule() ); // NOTE: Maven 3.8.1 used in demo has Sisu Index for ALL components (older Maven does NOT have!) binder.bind( ParameterKeys.PROPERTIES ).toInstance( System.getProperties() ); binder.bind( ShutdownThread.class ).asEagerSingleton(); From b5d6d9d7080a4cbba828086094400b9c487d5316 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 13 Oct 2022 10:03:42 +0200 Subject: [PATCH 3/5] Improve javadoc --- .../java/org/eclipse/aether/RepositorySystemSession.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java b/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java index 1ce3a27df..9cdb4131d 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java @@ -302,8 +302,13 @@ public interface RepositorySystemSession extends AutoCloseable * Important: ideally it is the session creator who should be responsible to close the session. The "nested" * (delegating, wrapped) sessions {@link AbstractForwardingRepositorySystemSession} and alike) by default * (without overriding the {@link AbstractForwardingRepositorySystemSession#close()} method) are not able to close - * session, and it is the "recommended" behaviour as well. The session "copy" (using copy-constructor) creates - * a copy instance, that does NOT share "closed" (nor "read-only") state with original session. + * session, and it is the "recommended" behaviour as well. On the other hand, "nested" session may receive + * "on close" handler registration, but those registrations are passed to delegated session and will be invoked + * once the "top level" (delegated) session is closed. + *

    + * The session "copy" instances (created using copy-constructor) result in new session instance, that does NOT + * share "closed" (nor "read-only") state with original session, hence they should be treated as "top level" + * sessions as well. *

    * The recommended pattern for "top level" sessions is the usual try-with-resource: * From 4a1f6189ad8ec5bab9c29ffde2134d6a2920e24b Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Thu, 13 Oct 2022 10:08:32 +0200 Subject: [PATCH 4/5] More javadoc --- .../org/eclipse/aether/RepositorySystemSession.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java b/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java index 9cdb4131d..0c2bbac90 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java @@ -301,14 +301,15 @@ public interface RepositorySystemSession extends AutoCloseable *

    * Important: ideally it is the session creator who should be responsible to close the session. The "nested" * (delegating, wrapped) sessions {@link AbstractForwardingRepositorySystemSession} and alike) by default - * (without overriding the {@link AbstractForwardingRepositorySystemSession#close()} method) are not able to close - * session, and it is the "recommended" behaviour as well. On the other hand, "nested" session may receive - * "on close" handler registration, but those registrations are passed to delegated session and will be invoked + * (without overriding the {@link AbstractForwardingRepositorySystemSession#close()} method) are prevented to close + * session, and it is the "recommended" behaviour as well. On the other hand, "nested" session may receive new + * "on close" handler registrations, but those registrations are passed to delegated session, and will be invoked * once the "top level" (delegated) session is closed. *

    - * The session "copy" instances (created using copy-constructor) result in new session instance, that does NOT - * share "closed" (nor "read-only") state with original session, hence they should be treated as "top level" - * sessions as well. + * New session "copy" instances created using copy-constructor + * {@link DefaultRepositorySystemSession#DefaultRepositorySystemSession(RepositorySystemSession)} result in new, + * independent session instances, and they do NOT share states like "read-only", "closed" and "on close handlers" + * with the copied session. Hence, they should be treated as "top level" sessions as well. *

    * The recommended pattern for "top level" sessions is the usual try-with-resource: * From 3183347579c10f17f4126b6c8276687d401c7093 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Fri, 14 Oct 2022 18:30:24 +0200 Subject: [PATCH 5/5] PR comment --- .../java/org/eclipse/aether/DefaultRepositorySystemSession.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java b/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java index 9e105c633..4450487f6 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java @@ -922,7 +922,7 @@ public void close() exceptions.add( e ); } } - MultiRuntimeException.mayThrow( "session onClose handler failures", exceptions ); + MultiRuntimeException.mayThrow( "session on-close handler failures", exceptions ); } } }