You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2022/10/13 07:11:00 UTC
[jira] [Commented] (MRESOLVER-278) BREAKING: Session must be explicitly closed (and introduce onCloseHandlers)
[ https://issues.apache.org/jira/browse/MRESOLVER-278?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17616827#comment-17616827 ]
ASF GitHub Bot commented on MRESOLVER-278:
------------------------------------------
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 ...
> BREAKING: Session must be explicitly closed (and introduce onCloseHandlers)
> ---------------------------------------------------------------------------
>
> Key: MRESOLVER-278
> URL: https://issues.apache.org/jira/browse/MRESOLVER-278
> Project: Maven Resolver
> Issue Type: New Feature
> Components: Resolver
> Reporter: Tamas Cservenak
> Assignee: Tamas Cservenak
> Priority: Major
> Fix For: 1.9.0
>
>
> So far, in (vanilla) Maven, the lifecycle of session was on par with lifecycle of SISU container, as Maven does something like this:
> * boot, create container
> * create session
> * work
> * destroy container
> * exit JVM
> So, Maven execution is 1 session 1 container, are on par.
> This is not true for cases where container (and resolver components) are reused across several sessions, like mvnd does. Also, current code on master (named locks adapter) uses {{@PreDestroy}} to shut down adapters, that is invoked when container is shut down, while the adapters are created per-session. This means that long-living mvnd daemons will shut down the unused adapter only when daemon itself is shut down, even is session for which adapter was created is long gone/done.
> While Maven has "session scoped" notion, resolver has not. Hence, simplest and cleanest solution is to make RepositorySystemSession have a method to "close", denoting that "this session is done" (but should not be AutoCloseable or Closeable!). Also, if we can provide hooks for "onSessionClose", this resolves all the problems, as for example the adapter, that is created per session, could be now cleanly shut down at session end.
> One gotcha: this change implies a {*}breaking change for integrators of resolver{*}: integrator should make sure they close the session after they are done with it.
> Example changes needed for resolver users: [https://github.com/apache/maven/pull/822]
> The "pattern" to make use of this feature in resolver is following:
> * stuff something into session data, ideally using computeWhenAbsent
> * if absent, register a callback hook as well
> * in callback, get the stuffed thing from session and if present, do something with it
--
This message was sent by Atlassian Jira
(v8.20.10#820010)