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/12/27 15:10:57 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request, #232: [MRESOLVER-302] Introduce onSessionClose hooks
cstamas opened a new pull request, #232:
URL: https://github.com/apache/maven-resolver/pull/232
That integrators must integrate, to provide onSessionClosed hooks functionality to any component requiring it.
---
https://issues.apache.org/jira/browse/MRESOLVER-302
--
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] kwin commented on a diff in pull request #232: [MRESOLVER-302] Introduce onSessionClose hooks
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #232:
URL: https://github.com/apache/maven-resolver/pull/232#discussion_r1057922037
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystem.java:
##########
@@ -302,4 +302,39 @@ List<RemoteRepository> newResolutionRepositories( RepositorySystemSession sessio
* @since 1.9.0
*/
void shutdown();
+
+ /**
+ * Registers the session for lifecycle tracking: it marks that the passed in session instance is about to start.
+ * After this call it is possible to register "on close" handlers using
+ * {@link #addOnSessionEndedHandle(RepositorySystemSession, Runnable)} method that will execute once
+ * {@link #sessionEnded(RepositorySystemSession)} method was invoked.
+ * <p>
+ * <en>Same session instance can be started only once.</em>
Review Comment:
wrong start tag `en`. Affects multiple sources in this PR.
--
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 #232: [MRESOLVER-302] Introduce onSessionClose hooks
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #232:
URL: https://github.com/apache/maven-resolver/pull/232#discussion_r1057927629
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystem.java:
##########
@@ -302,4 +302,39 @@ List<RemoteRepository> newResolutionRepositories( RepositorySystemSession sessio
* @since 1.9.0
*/
void shutdown();
+
+ /**
+ * Registers the session for lifecycle tracking: it marks that the passed in session instance is about to start.
+ * After this call it is possible to register "on close" handlers using
+ * {@link #addOnSessionEndedHandle(RepositorySystemSession, Runnable)} method that will execute once
+ * {@link #sessionEnded(RepositorySystemSession)} method was invoked.
+ * <p>
+ * <en>Same session instance can be started only once.</em>
+ *
+ * @param session the session that is about to start, never {@code null}.
+ * @since TBD
+ */
+ void sessionStarted( RepositorySystemSession session );
+
+ /**
+ * Registers a handler to execute when this session ends. This method throws, if the passed in session instance
+ * was not passed to method {@link #sessionStarted(RepositorySystemSession)} beforehand.
+ *
+ * @param session the session for which the handler needs to be registered, never {@code null}.
+ * @param handler the handler, never {@code null}.
+ * @since TBD
+ */
+ void addOnSessionEndedHandle( RepositorySystemSession session, Runnable handler );
+
+ /**
+ * Signals to repository system that passed in session ended, it will not be used anymore. Repository system
+ * will invoke the registered handlers for this session, if any. This method throws if the passed in session
+ * instance was not passed to method {@link #sessionStarted(RepositorySystemSession)} beforehand.
+ * <p>
+ * <en>Same session instance can be ended only once.</em>
+ *
+ * @param session the session that just ended, never {@code null}.
+ * @since TBD
+ */
+ void sessionEnded( RepositorySystemSession session );
Review Comment:
This is the least painful way with 1.x. Problem is how session instances are created (by client code, using def ctor of DefaultRepositorySystemSession, no factory for it) and _derived_. Just look thru Maven and plugin sources as example. They are not "boxed" nor are hierarchical, you can get session A and have it "derived" (using copy ctor) into session B and then continue to use it (typical when local repo is overridden in plugins), etc. And then i did not even mention "forwarding" session...
So yes, ideally session would be:
* created by some factory (repo system, ideally?)
* closeable
But doing this would be very disruptive, so baby steps... leave these changes for 2.0
--
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] kwin commented on a diff in pull request #232: [MRESOLVER-302] Introduce onSessionClose hooks
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #232:
URL: https://github.com/apache/maven-resolver/pull/232#discussion_r1057923595
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystem.java:
##########
@@ -302,4 +302,39 @@ List<RemoteRepository> newResolutionRepositories( RepositorySystemSession sessio
* @since 1.9.0
*/
void shutdown();
+
+ /**
+ * Registers the session for lifecycle tracking: it marks that the passed in session instance is about to start.
+ * After this call it is possible to register "on close" handlers using
+ * {@link #addOnSessionEndedHandle(RepositorySystemSession, Runnable)} method that will execute once
+ * {@link #sessionEnded(RepositorySystemSession)} method was invoked.
+ * <p>
+ * <en>Same session instance can be started only once.</em>
+ *
+ * @param session the session that is about to start, never {@code null}.
Review Comment:
Why can’t this be transparently handled whenever RSS is created?
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystem.java:
##########
@@ -302,4 +302,39 @@ List<RemoteRepository> newResolutionRepositories( RepositorySystemSession sessio
* @since 1.9.0
*/
void shutdown();
+
+ /**
+ * Registers the session for lifecycle tracking: it marks that the passed in session instance is about to start.
+ * After this call it is possible to register "on close" handlers using
+ * {@link #addOnSessionEndedHandle(RepositorySystemSession, Runnable)} method that will execute once
+ * {@link #sessionEnded(RepositorySystemSession)} method was invoked.
+ * <p>
+ * <en>Same session instance can be started only once.</em>
+ *
+ * @param session the session that is about to start, never {@code null}.
+ * @since TBD
+ */
+ void sessionStarted( RepositorySystemSession session );
+
+ /**
+ * Registers a handler to execute when this session ends. This method throws, if the passed in session instance
+ * was not passed to method {@link #sessionStarted(RepositorySystemSession)} beforehand.
+ *
+ * @param session the session for which the handler needs to be registered, never {@code null}.
+ * @param handler the handler, never {@code null}.
+ * @since TBD
+ */
+ void addOnSessionEndedHandle( RepositorySystemSession session, Runnable handler );
+
+ /**
+ * Signals to repository system that passed in session ended, it will not be used anymore. Repository system
+ * will invoke the registered handlers for this session, if any. This method throws if the passed in session
+ * instance was not passed to method {@link #sessionStarted(RepositorySystemSession)} beforehand.
+ * <p>
+ * <en>Same session instance can be ended only once.</em>
+ *
+ * @param session the session that just ended, never {@code null}.
+ * @since TBD
+ */
+ void sessionEnded( RepositorySystemSession session );
Review Comment:
From a consumer perspective a `close()` method on RSS would be a lot easier and in addition allows to use
- try with resources
- semantic code analysis tools to emit a WARN when someone forgot to call close.
--
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] kwin commented on a diff in pull request #232: [MRESOLVER-302] Introduce onSessionClose hooks
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #232:
URL: https://github.com/apache/maven-resolver/pull/232#discussion_r1059306441
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystem.java:
##########
@@ -302,4 +302,39 @@ List<RemoteRepository> newResolutionRepositories( RepositorySystemSession sessio
* @since 1.9.0
*/
void shutdown();
+
+ /**
+ * Registers the session for lifecycle tracking: it marks that the passed in session instance is about to start.
+ * After this call it is possible to register "on close" handlers using
+ * {@link #addOnSessionEndedHandle(RepositorySystemSession, Runnable)} method that will execute once
+ * {@link #sessionEnded(RepositorySystemSession)} method was invoked.
+ * <p>
+ * <en>Same session instance can be started only once.</em>
+ *
+ * @param session the session that is about to start, never {@code null}.
+ * @since TBD
+ */
+ void sessionStarted( RepositorySystemSession session );
+
+ /**
+ * Registers a handler to execute when this session ends. This method throws, if the passed in session instance
+ * was not passed to method {@link #sessionStarted(RepositorySystemSession)} beforehand.
+ *
+ * @param session the session for which the handler needs to be registered, never {@code null}.
+ * @param handler the handler, never {@code null}.
+ * @since TBD
+ */
+ void addOnSessionEndedHandle( RepositorySystemSession session, Runnable handler );
+
+ /**
+ * Signals to repository system that passed in session ended, it will not be used anymore. Repository system
+ * will invoke the registered handlers for this session, if any. This method throws if the passed in session
+ * instance was not passed to method {@link #sessionStarted(RepositorySystemSession)} beforehand.
+ * <p>
+ * <en>Same session instance can be ended only once.</em>
+ *
+ * @param session the session that just ended, never {@code null}.
+ * @since TBD
+ */
+ void sessionEnded( RepositorySystemSession session );
Review Comment:
Understood, but the same uncertainty is now applicable to `sessionEnded(...)`. When should consumers call it? What about sessions created with https://maven.apache.org/resolver/apidocs/org/eclipse/aether/DefaultRepositorySystemSession.html#%3Cinit%3E(org.eclipse.aether.RepositorySystemSession)? What about forwarding sessions. The Javadocs need to be extended in this regard.
--
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
Re: [PR] [MRESOLVER-302] Introduce onSessionClose hooks [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas closed pull request #232: [MRESOLVER-302] Introduce onSessionClose hooks
URL: https://github.com/apache/maven-resolver/pull/232
--
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] kwin commented on a diff in pull request #232: [MRESOLVER-302] Introduce onSessionClose hooks
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #232:
URL: https://github.com/apache/maven-resolver/pull/232#discussion_r1059609246
##########
maven-resolver-api/src/main/java/org/eclipse/aether/RepositorySystem.java:
##########
@@ -302,4 +302,39 @@ List<RemoteRepository> newResolutionRepositories( RepositorySystemSession sessio
* @since 1.9.0
*/
void shutdown();
+
+ /**
+ * Registers the session for lifecycle tracking: it marks that the passed in session instance is about to start.
+ * After this call it is possible to register "on close" handlers using
+ * {@link #addOnSessionEndedHandle(RepositorySystemSession, Runnable)} method that will execute once
+ * {@link #sessionEnded(RepositorySystemSession)} method was invoked.
+ * <p>
+ * <en>Same session instance can be started only once.</em>
+ *
+ * @param session the session that is about to start, never {@code null}.
+ * @since TBD
+ */
+ void sessionStarted( RepositorySystemSession session );
+
+ /**
+ * Registers a handler to execute when this session ends. This method throws, if the passed in session instance
+ * was not passed to method {@link #sessionStarted(RepositorySystemSession)} beforehand.
+ *
+ * @param session the session for which the handler needs to be registered, never {@code null}.
+ * @param handler the handler, never {@code null}.
+ * @since TBD
+ */
+ void addOnSessionEndedHandle( RepositorySystemSession session, Runnable handler );
+
+ /**
+ * Signals to repository system that passed in session ended, it will not be used anymore. Repository system
+ * will invoke the registered handlers for this session, if any. This method throws if the passed in session
+ * instance was not passed to method {@link #sessionStarted(RepositorySystemSession)} beforehand.
+ * <p>
+ * <en>Same session instance can be ended only once.</em>
+ *
+ * @param session the session that just ended, never {@code null}.
+ * @since TBD
+ */
+ void sessionEnded( RepositorySystemSession session );
Review Comment:
Given all that I still think that a `close()` on `RepositorySystemSession` is preferable.
The different implementations need to implement that method differently:
- `DefaultRepositorySystemSession.close()` should really do the cleanup in case it has been created from scratch
- `DefaultRepositorySystemSession.close()` should be a noop in case it has been created via copy constructor
- `AbstractForwardingRepositorySystemSession.close()` should be a noop
Doing this should be fully backwards compatible and still expose an API which is much easier to digest.
--
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
Re: [PR] [MRESOLVER-302] Introduce onSessionClose hooks [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #232:
URL: https://github.com/apache/maven-resolver/pull/232#issuecomment-1802207404
Superseded by https://github.com/apache/maven-resolver/pull/357
--
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