Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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>
* <em>Same session instance can be started only once.</em>
*
* @param session the session that is about to start, never {@code null}.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can’t this be transparently handled whenever RSS is created?

* @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>
* <em>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 );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
* under the License.
*/

import org.eclipse.aether.RepositorySystemSession;

/**
* Lifecycle managing component for repository system.
*
Expand All @@ -41,4 +43,33 @@ public interface RepositorySystemLifecycle
* Throws if repository system is already shut down.
*/
void addOnSystemEndedHandler( Runnable handler );

/**
* Registers the session for lifecycle tracking: it marks that the passed in session instance is about to start.
* <p>
* <em>Same session instance can be started only once.</em>
*
* @since TBD
*/
void sessionStarted( RepositorySystemSession session );

/**
* Signals that passed in session was 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>
* <em>Same session instance can be ended only once.</em>
*
* @since TBD
*/
void sessionEnded( RepositorySystemSession session );

/**
* Registers an "on session end" handler.
* <p>
* Throws if session was not passed to {@link #sessionStarted(RepositorySystemSession)} beforehand.
*
* @since TBD
*/
void addOnSessionEndedHandle( RepositorySystemSession session, Runnable handler );
}
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,27 @@ public void shutdown()
}
}

@Override
public void sessionStarted( RepositorySystemSession session )
{
validateSession( session );
repositorySystemLifecycle.sessionStarted( session );
}

@Override
public void sessionEnded( RepositorySystemSession session )
{
validateSession( session );
repositorySystemLifecycle.sessionEnded( session );
}

@Override
public void addOnSessionEndedHandle( RepositorySystemSession session, Runnable handler )
{
validateSession( session );
repositorySystemLifecycle.addOnSessionEndedHandle( session, handler );
}

private void validateSession( RepositorySystemSession session )
{
requireNonNull( session, "repository system session cannot be null" );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,13 @@
import javax.inject.Singleton;

import java.util.ArrayList;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;

import org.eclipse.aether.MultiRuntimeException;
import org.eclipse.aether.RepositorySystemSession;
import org.eclipse.aether.impl.RepositorySystemLifecycle;

import static java.util.Objects.requireNonNull;
Expand All @@ -40,15 +43,23 @@
public class DefaultRepositorySystemLifecycle
implements RepositorySystemLifecycle
{
private static final String LIFECYCLE_ID_KEY = DefaultRepositorySystemLifecycle.class + ".key";

private final AtomicBoolean shutdown;

private final CopyOnWriteArrayList<Runnable> onSystemEndedHandlers;

private final ConcurrentHashMap<String, CopyOnWriteArrayList<Runnable>> onSessionEndedHandlers;

private final AtomicInteger sessionIdCounter;

@Inject
public DefaultRepositorySystemLifecycle()
{
this.shutdown = new AtomicBoolean( false );
this.onSystemEndedHandlers = new CopyOnWriteArrayList<>();
this.onSessionEndedHandlers = new ConcurrentHashMap<>();
this.sessionIdCounter = new AtomicInteger( 0 );
}

@Override
Expand Down Expand Up @@ -80,11 +91,85 @@ public void addOnSystemEndedHandler( Runnable handler )
onSystemEndedHandlers.add( 0, handler );
}

@Override
public void sessionStarted( RepositorySystemSession session )
{
requireNonNull( session, "session cannot be null" );
requireNotShutdown();
String sessionId = sessionId( session );
onSessionEndedHandlers.compute( sessionId, ( k, v ) ->
{
if ( v != null )
{
throw new IllegalStateException( "session instance already registered" );
}
return new CopyOnWriteArrayList<>();
} );
}

@Override
public void sessionEnded( RepositorySystemSession session )
{
requireNonNull( session, "session cannot be null" );
requireNotShutdown();
String sessionId = sessionId( session );
ArrayList<Runnable> handlers = new ArrayList<>();
onSessionEndedHandlers.compute( sessionId, ( k, v ) ->
{
if ( v == null )
{
throw new IllegalStateException( "session instance not registered" );
}
handlers.addAll( v );
return null;
} );

ArrayList<Exception> exceptions = new ArrayList<>();
for ( Runnable handler : handlers )
{
try
{
handler.run();
}
catch ( Exception e )
{
exceptions.add( e );
}
}
MultiRuntimeException.mayThrow( "sessionEnded handler issue(s)", exceptions );
}

@Override
public void addOnSessionEndedHandle( RepositorySystemSession session, Runnable handler )
{
requireNonNull( session, "session cannot be null" );
requireNonNull( handler, "handler cannot be null" );
requireNotShutdown();
String sessionId = sessionId( session );
onSessionEndedHandlers.compute( sessionId, ( k, v ) ->
{
if ( v == null )
{
throw new IllegalStateException( "session instance not registered" );
}
v.add( handler );
return v;
} );
}

private void requireNotShutdown()
{
if ( shutdown.get() )
{
throw new IllegalStateException( "repository system is already shut down" );
}
}

private String sessionId( RepositorySystemSession session )
{
String id = (String) session.getData()
.computeIfAbsent( LIFECYCLE_ID_KEY, () -> String.valueOf( sessionIdCounter.incrementAndGet() ) );
id += "-" + System.identityHashCode( session );
return id;
}
}