You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/10/11 10:47:22 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request, #201: Closeable session
cstamas opened a new pull request, #201:
URL: https://github.com/apache/maven-resolver/pull/201
Removes use of `@PreDestroy` and is able to properly hook into session to perform various tasks when session is closed. Two examples converted, the named locks sync context factory and the summary checksums save.
Problem: this REQUIRES host (resolver integrator) change, as session MUST be closed to have these actions performed.
--
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
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r996098322
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +888,41 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> handler )
+ {
+ verifyStateForMutation();
+ requireNonNull( handler, "handler cannot be null" );
+ onCloseHandlers.add( 0, handler );
Review Comment:
OK, you you prepend to avoid the reverse iteration at end?
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r994267731
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +889,44 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> 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<Exception> exceptions = new ArrayList<>();
+ ListIterator<Consumer<RepositorySystemSession>> handlerIterator
+ = onCloseHandlers.listIterator( onCloseHandlers.size() );
+ while ( handlerIterator.hasPrevious() )
Review Comment:
Agreed and fixed
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995933797
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -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<Path, Set<String>> recordedLines;
+
+ private SummaryFileWriter( Path basedir,
+ boolean originAware,
+ ConcurrentHashMap<Path, Set<String>> recordedLines )
Review Comment:
To avoid duplications, as artifacts like plexus-utils are going thru resolver many times... (in essence, it uses set while "accumulating" to avoid duplicates which is better for memory as well, as in gather all in a list, and dedupe at save)
--
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
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r996096483
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java:
##########
@@ -271,4 +272,57 @@
@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<RepositorySystemSession> 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<RepositorySystemSession>)} method. This method may be invoked multiple times,
+ * but only first invocation will actually invoke handlers, subsequent invocations will be no-op.
+ * <p>
+ * 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.
+ * <p>
+ * 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 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.
+ * <p>
+ * 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.
+ * <p>
+ * The recommended pattern for "top level" sessions is the usual try-with-resource:
+ *
+ * <pre> {@code
+ * try ( RepositorySystemSession session = create session... )
Review Comment:
Indeed, thank you!
--
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
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995866800
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java:
##########
@@ -271,4 +272,57 @@
@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<RepositorySystemSession> 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<RepositorySystemSession>)} method. This method may be invoked multiple times,
+ * but only first invocation will actually invoke handlers, subsequent invocations will be no-op.
+ * <p>
+ * 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.
+ * <p>
+ * 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 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.
+ * <p>
+ * 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.
+ * <p>
+ * The recommended pattern for "top level" sessions is the usual try-with-resource:
+ *
+ * <pre> {@code
+ * try ( RepositorySystemSession session = create session... )
Review Comment:
I disagree, the scope is well defined in maven:
https://github.com/apache/maven/blob/ae655e0e83e47ec7389763bcabe9d84e92862179/maven-core/src/main/java/org/apache/maven/DefaultMaven.java#L234-L253
--
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
[GitHub] [maven-resolver] cstamas merged pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas merged PR #201:
URL: https://github.com/apache/maven-resolver/pull/201
--
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
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r994220780
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +889,44 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> 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<Exception> exceptions = new ArrayList<>();
+ ListIterator<Consumer<RepositorySystemSession>> handlerIterator
+ = onCloseHandlers.listIterator( onCloseHandlers.size() );
+ while ( handlerIterator.hasPrevious() )
Review Comment:
I kinda dislike using iterators...
Could we add them in reverse order ?
```
// add the handler to the beginning of the list so that the last handler will be called first
onCloseHandlers.add( 0, handler );
```
and then
```
for ( Consumer<RepositorySystemSession> handler : onCloseHandlers )
{
...
}
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java:
##########
@@ -271,4 +272,51 @@
@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<RepositorySystemSession> 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<RepositorySystemSession>)} method. This method may be invoked multiple times,
+ * but only first invocation will actually invoke handlers, subsequent invocations will be no-op.
+ * <p>
+ * 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.
+ * <p>
+ * 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:
+ *
+ * <pre> {@code
+ * RepositorySystemSession session = create session...
+ * try
+ * {
+ * ... use/nest session
+ * }
+ * finally
+ * {
+ * session.close();
+ * }
+ * }</pre>
+ *
+ * @since TBD
+ */
+ void close();
Review Comment:
If that's really the intended pattern, shouldn't `Session` extends `AutoCloseable` ?
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java:
##########
@@ -271,4 +272,51 @@
@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<RepositorySystemSession> 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<RepositorySystemSession>)} method. This method may be invoked multiple times,
+ * but only first invocation will actually invoke handlers, subsequent invocations will be no-op.
+ * <p>
+ * 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.
+ * <p>
+ * 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:
Review Comment:
Are they really able to close session ? It seems they throw an exception...
##########
maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/sisu/SisuRepositorySystemDemoModule.java:
##########
@@ -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!)
Review Comment:
Do we need to keep that commented line ?
##########
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<? extends Throwable> throwable;
Review Comment:
`throwables` ? with an `s`
Also in the getter / parameter ...
--
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
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r996099460
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +888,41 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> handler )
+ {
+ verifyStateForMutation();
+ requireNonNull( handler, "handler cannot be null" );
+ onCloseHandlers.add( 0, handler );
Review Comment:
clear
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r996098795
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +888,41 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> handler )
+ {
+ verifyStateForMutation();
+ requireNonNull( handler, "handler cannot be null" );
+ onCloseHandlers.add( 0, handler );
Review Comment:
yup
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995934236
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -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<Path, Set<String>> recordedLines;
+
+ private SummaryFileWriter( Path basedir,
+ boolean originAware,
+ ConcurrentHashMap<Path, Set<String>> recordedLines )
{
this.basedir = basedir;
this.originAware = originAware;
+ this.recordedLines = recordedLines;
}
@Override
public void addTrustedArtifactChecksums( Artifact artifact, ArtifactRepository artifactRepository,
List<ChecksumAlgorithmFactory> checksumAlgorithmFactories,
- Map<String, String> trustedArtifactChecksums ) throws IOException
+ Map<String, String> 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 );
Review Comment:
Sorted is really just a "nice to have" thing, is easier to author/grasp "recorded" file
--
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
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995792085
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -245,4 +255,35 @@ public void close()
// nop
}
}
+
+ @SuppressWarnings( "unchecked" )
+ private void saveSessionRecordedLines( RepositorySystemSession session )
+ {
+ Map<Path, Set<String>> recordedLines = (Map<Path, Set<String>>) session.getData().get( RECORDING_KEY );
+ if ( recordedLines == null || recordedLines.isEmpty() )
+ {
+ return;
+ }
+
+ ArrayList<Exception> exceptions = new ArrayList<>();
+ for ( Map.Entry<Path, Set<String>> entry : recordedLines.entrySet() )
+ {
+ Path file = entry.getKey();
+ Set<String> lines = entry.getValue();
+ if ( !lines.isEmpty() )
+ {
+ LOGGER.info( "Saving {} checksums to '{}'", lines.size(), file );
+ try
+ {
+ Files.createDirectories( file.getParent() );
+ Files.write( file, lines );
Review Comment:
This is now truly ONE file per checksum algo?
##########
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 );
+ }
Review Comment:
This is an unrelated cleanup, no?
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +888,41 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> handler )
+ {
+ verifyStateForMutation();
+ requireNonNull( handler, "handler cannot be null" );
+ onCloseHandlers.add( 0, handler );
+ }
+
+ @Override
+ public boolean isClosed()
+ {
+ return closed.get();
+ }
+
+ @Override
+ public void close()
+ {
+ if ( closed.compareAndSet( false, true ) )
+ {
+ ArrayList<Exception> exceptions = new ArrayList<>();
+ for ( Consumer<RepositorySystemSession> onCloseHandler : onCloseHandlers )
+ {
+ try
+ {
+ onCloseHandler.accept( this );
+ }
+ catch ( Exception e )
+ {
+ exceptions.add( e );
+ }
+ }
+ MultiRuntimeException.mayThrow( "session onClose handler failures", exceptions );
Review Comment:
on-close
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +888,41 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> handler )
+ {
+ verifyStateForMutation();
+ requireNonNull( handler, "handler cannot be null" );
+ onCloseHandlers.add( 0, handler );
Review Comment:
Shouldn't this be just like with locks: FILO?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -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<Path, Set<String>> recordedLines;
+
+ private SummaryFileWriter( Path basedir,
+ boolean originAware,
+ ConcurrentHashMap<Path, Set<String>> recordedLines )
{
this.basedir = basedir;
this.originAware = originAware;
+ this.recordedLines = recordedLines;
}
@Override
public void addTrustedArtifactChecksums( Artifact artifact, ArtifactRepository artifactRepository,
List<ChecksumAlgorithmFactory> checksumAlgorithmFactories,
- Map<String, String> trustedArtifactChecksums ) throws IOException
+ Map<String, String> 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 );
Review Comment:
Do we really need it sorted or will insertion order just suffice?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/DefaultSyncContextFactory.java:
##########
@@ -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();
+ }
Review Comment:
So the adapter showdown is now handled by the AutoCloseable for each?
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java:
##########
@@ -271,4 +272,57 @@
@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<RepositorySystemSession> 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<RepositorySystemSession>)} method. This method may be invoked multiple times,
+ * but only first invocation will actually invoke handlers, subsequent invocations will be no-op.
+ * <p>
+ * 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.
+ * <p>
+ * 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 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.
+ * <p>
+ * 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.
+ * <p>
+ * The recommended pattern for "top level" sessions is the usual try-with-resource:
+ *
+ * <pre> {@code
+ * try ( RepositorySystemSession session = create session... )
Review Comment:
This confuses me. In Maven the repo session is passed around like a whore, how can this be achived in Maven? It can't, no?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -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<Path, Set<String>> recordedLines;
+
+ private SummaryFileWriter( Path basedir,
+ boolean originAware,
+ ConcurrentHashMap<Path, Set<String>> recordedLines )
Review Comment:
Why do we need the semantics of a set?
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r994268461
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystemSession.java:
##########
@@ -271,4 +272,51 @@
@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<RepositorySystemSession> 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<RepositorySystemSession>)} method. This method may be invoked multiple times,
+ * but only first invocation will actually invoke handlers, subsequent invocations will be no-op.
+ * <p>
+ * 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.
+ * <p>
+ * 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:
Review Comment:
Tried to clarify, please re-read.
--
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
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r996096735
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -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<Path, Set<String>> recordedLines;
+
+ private SummaryFileWriter( Path basedir,
+ boolean originAware,
+ ConcurrentHashMap<Path, Set<String>> recordedLines )
Review Comment:
makes sense
--
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
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995868188
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -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<Path, Set<String>> recordedLines;
+
+ private SummaryFileWriter( Path basedir,
+ boolean originAware,
+ ConcurrentHashMap<Path, Set<String>> recordedLines )
{
this.basedir = basedir;
this.originAware = originAware;
+ this.recordedLines = recordedLines;
}
@Override
public void addTrustedArtifactChecksums( Artifact artifact, ArtifactRepository artifactRepository,
List<ChecksumAlgorithmFactory> checksumAlgorithmFactories,
- Map<String, String> trustedArtifactChecksums ) throws IOException
+ Map<String, String> 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 );
Review Comment:
I think it will directly affect the output when written. So if it's done on the fly, it will have to be done before writing the results.
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995932933
##########
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 );
+ }
Review Comment:
No, is related: as session got new method, all methods so far existing on interface had no params (but addOnCloseHandler have 1 param)
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995932176
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +888,41 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> handler )
+ {
+ verifyStateForMutation();
+ requireNonNull( handler, "handler cannot be null" );
+ onCloseHandlers.add( 0, handler );
Review Comment:
It is FILO, or what do you mean? List is populated in "reverse" order while processed (handlers triggered) in "normal" order (from apsect of the List) => FILO - last triggered handler is first added.
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995936078
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/DefaultSyncContextFactory.java:
##########
@@ -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();
+ }
Review Comment:
The adapter shutdown is now handled by session onCloseHandler (as since https://github.com/apache/maven-resolver/commit/23197f4c50abd8d553059c4f24e2e6d35d015310 commit they are created per-session).
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995934644
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -245,4 +255,35 @@ public void close()
// nop
}
}
+
+ @SuppressWarnings( "unchecked" )
+ private void saveSessionRecordedLines( RepositorySystemSession session )
+ {
+ Map<Path, Set<String>> recordedLines = (Map<Path, Set<String>>) session.getData().get( RECORDING_KEY );
+ if ( recordedLines == null || recordedLines.isEmpty() )
+ {
+ return;
+ }
+
+ ArrayList<Exception> exceptions = new ArrayList<>();
+ for ( Map.Entry<Path, Set<String>> entry : recordedLines.entrySet() )
+ {
+ Path file = entry.getKey();
+ Set<String> lines = entry.getValue();
+ if ( !lines.isEmpty() )
+ {
+ LOGGER.info( "Saving {} checksums to '{}'", lines.size(), file );
+ try
+ {
+ Files.createDirectories( file.getParent() );
+ Files.write( file, lines );
Review Comment:
yes, a files are named as checksums-${repo.id}.${checksum.ext}
--
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
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #201: [MRESOLVER-278] Session close and onClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #201:
URL: https://github.com/apache/maven-resolver/pull/201#discussion_r995932176
##########
maven-resolver-api/src/main/java/org/eclipse/aether/DefaultRepositorySystemSession.java:
##########
@@ -873,4 +888,41 @@ public Collection<FileTransformer> getTransformersForArtifact( Artifact artifact
}
}
+ private final CopyOnWriteArrayList<Consumer<RepositorySystemSession>> onCloseHandlers
+ = new CopyOnWriteArrayList<>();
+
+ @Override
+ public void addOnCloseHandler( Consumer<RepositorySystemSession> handler )
+ {
+ verifyStateForMutation();
+ requireNonNull( handler, "handler cannot be null" );
+ onCloseHandlers.add( 0, handler );
Review Comment:
It is FILO, or what do you mean? List is populated in "reverse" order while processed (handlers triggered) in "normal" order => FILO - last triggered handler is first added.
--
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