gnodet commented on code in PR #357:
URL: https://github.com/apache/maven-resolver/pull/357#discussion_r1386866973
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystem.java:
##########
@@ -304,8 +314,26 @@ List<RemoteRepository> newResolutionRepositories(
* When shutdown happens, all the registered on-close 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>
+ * Note: this method actually just calls {@link #close()}.
*
* @since 1.9.0
+ * @deprecated Use {@link #close()} instead.
*/
+ @Deprecated
void shutdown();
+
+ /**
+ * Signals to repository system to shut down. Shut down instance is not
usable anymore.
+ * <p>
+ * Repository system may perform some resource cleanup, if applicable. Not
using this method may cause leaks or
+ * unclean shutdown of some subsystem.
+ * <p>
+ * When shutdown happens, all the registered on-close 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.
+ *
+ * @since TBD
+ */
+ void close();
Review Comment:
Do we want:
```
default void close() {
shutdown();
}
```
This would help with compatibility, but I don't think anyone is
_implementing_ this interface, right ? Maybe wrapped ?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemLifecycle.java:
##########
@@ -69,6 +74,59 @@ public void addOnSystemEndedHandler(Runnable handler) {
onSystemEndedHandlers.add(0, handler);
}
+ @Override
+ public void sessionStarted(CloseableRepositorySystemSession session) {
+ requireNonNull(session, "session cannot be null");
+ requireNotShutdown();
+ String sessionId = session.sessionId();
+ onSessionEndedHandlers.compute(sessionId, (k, v) -> {
+ if (v != null) {
+ throw new IllegalStateException("session instance already
registered");
+ }
+ return new CopyOnWriteArrayList<>();
+ });
+ }
+
+ @Override
+ public void sessionEnded(CloseableRepositorySystemSession session) {
+ requireNonNull(session, "session cannot be null");
+ requireNotShutdown();
+ String sessionId = session.sessionId();
+ ArrayList<Runnable> handlers = new ArrayList<>();
+ onSessionEndedHandlers.compute(sessionId, (k, v) -> {
+ if (v == null) {
+ throw new IllegalStateException("session instance not
registered");
+ }
+ handlers.addAll(v);
Review Comment:
Why not `handlers = v;` ? and initialize `handlers =
Collections.emptyList()` ?
##########
maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkHttpTransporter.java:
##########
@@ -417,7 +421,14 @@ protected PasswordAuthentication
getPasswordAuthentication() {
});
}
- return builder.build();
+ HttpClient result = builder.build();
+ // Only possible in Java21
Review Comment:
What's the plan here ? Should that be compiled with JDK 21 and with a check
on the JDK used at runtime ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
