Skip to content

[MRESOLVER-153] Make TrackingFileManager use NamedLocks #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

[MRESOLVER-153] Make TrackingFileManager use NamedLocks #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
968d2db
[MRESOLVER-153] Make TrackingFileManager use NamedLocks
cstamas Apr 27, 2021
0e254bb
Pull out NamedLockFactory selection and expose selected one.
cstamas Apr 27, 2021
153c81b
Step3: tie up things
cstamas Apr 27, 2021
3f1a831
Step4: use locking
cstamas Apr 27, 2021
c3ed127
Drop noop namemapeer and replace it with nooplock
cstamas Apr 27, 2021
4b1b096
Update doco
cstamas Apr 27, 2021
f9cb618
Hash file canonical path to form lock name
cstamas Apr 27, 2021
0abbe33
Make new NamedFactorySelector Guice managed.
cstamas Apr 27, 2021
dac6f97
PR comments, typos, tidy
cstamas Apr 27, 2021
ce21247
Align messages
cstamas Apr 28, 2021
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
2 changes: 2 additions & 0 deletions maven-resolver-impl/src/main/java/org/eclipse/aether/impl/DefaultServiceLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.eclipse.aether.internal.impl.SimpleLocalRepositoryManagerFactory;
import org.eclipse.aether.internal.impl.slf4j.Slf4jLoggerFactory;
import org.eclipse.aether.internal.impl.synccontext.DefaultSyncContextFactory;
import org.eclipse.aether.internal.impl.synccontext.NamedLockFactorySelector;
import org.eclipse.aether.spi.connector.checksum.ChecksumPolicyProvider;
import org.eclipse.aether.spi.connector.layout.RepositoryLayoutFactory;
import org.eclipse.aether.spi.connector.layout.RepositoryLayoutProvider;
Expand Down Expand Up @@ -221,6 +222,7 @@ public DefaultServiceLocator()
addService( LocalRepositoryManagerFactory.class, EnhancedLocalRepositoryManagerFactory.class );
addService( LoggerFactory.class, Slf4jLoggerFactory.class );
addService( TrackingFileManager.class, DefaultTrackingFileManager.class );
addService( NamedLockFactorySelector.class, NamedLockFactorySelector.class );
}

private <T> Entry<T> getEntry( Class<T> type, boolean create )
Expand Down
33 changes: 7 additions & 26 deletions maven-resolver-impl/src/main/java/org/eclipse/aether/impl/guice/AetherModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,7 @@
import org.eclipse.aether.internal.impl.DefaultTrackingFileManager;
import org.eclipse.aether.internal.impl.TrackingFileManager;
import org.eclipse.aether.internal.impl.synccontext.DefaultSyncContextFactory;
import org.eclipse.aether.internal.impl.synccontext.GlobalSyncContextFactory;
import org.eclipse.aether.internal.impl.synccontext.NamedSyncContextFactory;
import org.eclipse.aether.internal.impl.synccontext.NoLockSyncContextFactory;
import org.eclipse.aether.internal.impl.synccontext.SyncContextFactoryDelegate;
import org.eclipse.aether.internal.impl.synccontext.NamedLockFactorySelector;
import org.eclipse.aether.internal.impl.synccontext.named.GAVNameMapper;
import org.eclipse.aether.internal.impl.synccontext.named.DiscriminatingNameMapper;
import org.eclipse.aether.internal.impl.synccontext.named.NameMapper;
Expand Down Expand Up @@ -77,6 +74,7 @@
import org.eclipse.aether.internal.impl.Maven2RepositoryLayoutFactory;
import org.eclipse.aether.internal.impl.SimpleLocalRepositoryManagerFactory;
import org.eclipse.aether.internal.impl.slf4j.Slf4jLoggerFactory;
import org.eclipse.aether.named.providers.NoopNamedLockFactory;
import org.eclipse.aether.spi.connector.checksum.ChecksumPolicyProvider;
import org.eclipse.aether.spi.connector.layout.RepositoryLayoutFactory;
import org.eclipse.aether.spi.connector.layout.RepositoryLayoutProvider;
Expand Down Expand Up @@ -160,16 +158,11 @@ protected void configure()
.to( EnhancedLocalRepositoryManagerFactory.class ).in( Singleton.class );
bind( TrackingFileManager.class ).to( DefaultTrackingFileManager.class ).in( Singleton.class );

bind( NamedLockFactorySelector.class ).in( Singleton.class );
bind( SyncContextFactory.class ).to( DefaultSyncContextFactory.class ).in( Singleton.class );
bind( org.eclipse.aether.impl.SyncContextFactory.class )
.to( org.eclipse.aether.internal.impl.synccontext.legacy.DefaultSyncContextFactory.class )
.in( Singleton.class );
bind( SyncContextFactoryDelegate.class ).annotatedWith( Names.named( NoLockSyncContextFactory.NAME ) )
.to( NoLockSyncContextFactory.class ).in( Singleton.class );
bind( SyncContextFactoryDelegate.class ).annotatedWith( Names.named( GlobalSyncContextFactory.NAME ) )
.to( GlobalSyncContextFactory.class ).in( Singleton.class );
bind( SyncContextFactoryDelegate.class ).annotatedWith( Names.named( NamedSyncContextFactory.NAME ) )
.to( NamedSyncContextFactory.class ).in( Singleton.class );

bind( NameMapper.class ).annotatedWith( Names.named( StaticNameMapper.NAME ) )
.to( StaticNameMapper.class ).in( Singleton.class );
Expand All @@ -178,29 +171,17 @@ protected void configure()
bind( NameMapper.class ).annotatedWith( Names.named( DiscriminatingNameMapper.NAME ) )
.to( DiscriminatingNameMapper.class ).in( Singleton.class );

bind( NamedLockFactory.class ).annotatedWith( Names.named( NoopNamedLockFactory.NAME ) )
.to( NoopNamedLockFactory.class ).in( Singleton.class );
bind( NamedLockFactory.class ).annotatedWith( Names.named( LocalReadWriteLockNamedLockFactory.NAME ) )
.to( LocalReadWriteLockNamedLockFactory.class ).in( Singleton.class );
.to( LocalReadWriteLockNamedLockFactory.class ).in( Singleton.class );
bind( NamedLockFactory.class ).annotatedWith( Names.named( LocalSemaphoreNamedLockFactory.NAME ) )
.to( LocalSemaphoreNamedLockFactory.class ).in( Singleton.class );
.to( LocalSemaphoreNamedLockFactory.class ).in( Singleton.class );

install( new Slf4jModule() );

}

@Provides
@Singleton
Map<String, SyncContextFactoryDelegate> provideSyncContextFactoryDelegates(
@Named( NoLockSyncContextFactory.NAME ) SyncContextFactoryDelegate nolock,
@Named( GlobalSyncContextFactory.NAME ) SyncContextFactoryDelegate global,
@Named( NamedSyncContextFactory.NAME ) SyncContextFactoryDelegate named )
{
Map<String, SyncContextFactoryDelegate> factories = new HashMap<>();
factories.put( NoLockSyncContextFactory.NAME, nolock );
factories.put( GlobalSyncContextFactory.NAME, global );
factories.put( NamedSyncContextFactory.NAME, named );
return Collections.unmodifiableMap( factories );
}

@Provides
@Singleton
Map<String, NameMapper> provideNameMappers(
Expand Down
211 changes: 144 additions & 67 deletions ...olver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,18 @@
import java.io.IOException;
import java.io.RandomAccessFile;
import java.util.Map;
import java.util.Objects;
import java.util.Properties;

import javax.inject.Inject;
import javax.inject.Named;
import javax.inject.Singleton;

import org.eclipse.aether.internal.impl.synccontext.NamedLockFactorySelector;
import org.eclipse.aether.named.NamedLock;
import org.eclipse.aether.named.NamedLockFactory;
import org.eclipse.aether.spi.locator.Service;
import org.eclipse.aether.spi.locator.ServiceLocator;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -41,100 +48,170 @@
@Singleton
@Named
public final class DefaultTrackingFileManager
implements TrackingFileManager
implements TrackingFileManager, Service
{
private static final Logger LOGGER = LoggerFactory.getLogger( DefaultTrackingFileManager.class );

private static final String LOCK_PREFIX = "tracking:";

private NamedLockFactory namedLockFactory;

public DefaultTrackingFileManager()
{
// ctor for ServiceLocator
}

@Inject
public DefaultTrackingFileManager( final NamedLockFactorySelector selector )
{
this.namedLockFactory = selector.getSelectedNamedLockFactory();
}

@Override
public void initService( final ServiceLocator locator )
{
NamedLockFactorySelector select = Objects.requireNonNull(
locator.getService( NamedLockFactorySelector.class ) );
this.namedLockFactory = select.getSelectedNamedLockFactory();
}

private String getFileKey( final File file )
{
return LOCK_PREFIX + file.getAbsolutePath();
}

@Override
public Properties read( File file )
{
FileInputStream stream = null;
try
try ( NamedLock lock = namedLockFactory.getLock( getFileKey( file ) ) )
Copy link
Member

Choose a reason for hiding this comment

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

This suffers now from one problem. Since we don't use name mappers you will have the SAME lock name on two different CI nodes with Redisson using the same directory structure. They will block each other for no reason. Makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

Use selector here?

Copy link
Member Author

Choose a reason for hiding this comment

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

on two different nodes using two different storage mounts (disks), why'd you use redisson to sync those two (unrelated) nodes at all?

Copy link
Member

Choose a reason for hiding this comment

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

My point is that I expect consistent approach to name mapping wherever the lock factory is used. I need to think about this more tomorrow.

{
if ( !file.exists() )
if ( lock.lockShared( NamedLockFactorySelector.TIME, NamedLockFactorySelector.TIME_UNIT ) )
{
return null;
try
{
FileInputStream stream = null;
try
{
if ( !file.exists() )
{
return null;
}

stream = new FileInputStream( file );

Properties props = new Properties();
props.load( stream );

return props;
}
catch ( IOException e )
{
LOGGER.warn( "Failed to read tracking file {}", file, e );
}
finally
{
close( stream, file );
}

return null;
}
finally
{
lock.unlock();
}
}
else
{
throw new IllegalStateException( "Could not acquire read lock for: " + file );
}

stream = new FileInputStream( file );

Properties props = new Properties();
props.load( stream );

return props;
}
catch ( IOException e )
{
LOGGER.warn( "Failed to read tracking file {}", file, e );
}
finally
catch ( InterruptedException e )
{
close( stream, file );
throw new IllegalStateException( e );
}

return null;
}

@Override
public Properties update( File file, Map<String, String> updates )
{
Properties props = new Properties();

File directory = file.getParentFile();
if ( !directory.mkdirs() && !directory.exists() )
{
LOGGER.warn( "Failed to create parent directories for tracking file {}", file );
return props;
}

RandomAccessFile raf = null;
try
try ( NamedLock lock = namedLockFactory.getLock( getFileKey( file ) ) )
{
raf = new RandomAccessFile( file, "rw" );

if ( file.canRead() )
if ( lock.lockExclusively( NamedLockFactorySelector.TIME, NamedLockFactorySelector.TIME_UNIT ) )
{
byte[] buffer = new byte[(int) raf.length()];

raf.readFully( buffer );

ByteArrayInputStream stream = new ByteArrayInputStream( buffer );

props.load( stream );
}

for ( Map.Entry<String, String> update : updates.entrySet() )
{
if ( update.getValue() == null )
try
{
props.remove( update.getKey() );
Properties props = new Properties();

File directory = file.getParentFile();
if ( !directory.mkdirs() && !directory.exists() )
{
LOGGER.warn( "Failed to create parent directories for tracking file {}", file );
return props;
}

RandomAccessFile raf = null;
try
{
raf = new RandomAccessFile( file, "rw" );

if ( file.canRead() )
{
byte[] buffer = new byte[(int) raf.length()];

raf.readFully( buffer );

ByteArrayInputStream stream = new ByteArrayInputStream( buffer );

props.load( stream );
}

for ( Map.Entry<String, String> update : updates.entrySet() )
{
if ( update.getValue() == null )
{
props.remove( update.getKey() );
}
else
{
props.setProperty( update.getKey(), update.getValue() );
}
}

ByteArrayOutputStream stream = new ByteArrayOutputStream( 1024 * 2 );

LOGGER.debug( "Writing tracking file {}", file );
props.store( stream, "NOTE: This is a Maven Resolver internal implementation file"
+ ", its format can be changed without prior notice." );

raf.seek( 0 );
raf.write( stream.toByteArray() );
raf.setLength( raf.getFilePointer() );
}
catch ( IOException e )
{
LOGGER.warn( "Failed to write tracking file {}", file, e );
}
finally
{
close( raf, file );
}

return props;
}
else
finally
{
props.setProperty( update.getKey(), update.getValue() );
lock.unlock();
}
}

ByteArrayOutputStream stream = new ByteArrayOutputStream( 1024 * 2 );

LOGGER.debug( "Writing tracking file {}", file );
props.store( stream, "NOTE: This is a Maven Resolver internal implementation file"
+ ", its format can be changed without prior notice." );

raf.seek( 0 );
raf.write( stream.toByteArray() );
raf.setLength( raf.getFilePointer() );
}
catch ( IOException e )
{
LOGGER.warn( "Failed to write tracking file {}", file, e );
else
{
throw new IllegalStateException( "Could not acquire write lock for: " + file );
}
}
finally
catch ( InterruptedException e )
{
close( raf, file );
throw new IllegalStateException( e );
}

return props;
}

private void close( Closeable closeable, File file )
Expand Down
Loading