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 2020/07/10 07:37:47 UTC
[GitHub] [maven-resolver] michael-o opened a new pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
michael-o opened a new pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456885102
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
@Tibor17 Please read the API of `SyncContextFactory`. Sync contexts are never shared, they are thread private and everything runs in try-with-resources clauses. This is the contract. If you don't stick to do, the behavior is undefined. The current code as well as the change stick to it. The count is necessary to release reentrant acquired locks. I don't see how a `CountDownLatch` would help here. I agree with @cstamas . It serves a completely different purpose. I read the `SyncContextFactory` documentation at least five times to understand what I expected from an implementation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661477498
Then why we have the locks due to nothing is concurrent?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456886247
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
@michael-o
@cstamas
One way or another this code cannot be like this. If we showed this to the Oracle guys especially from the Java Concurrency group, they would say that this is dangerous code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r453207682
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.debug( "Acquiring global {} lock (currently held: {})",
Review comment:
I consider them being very helpful. I someone reports an error with this feature, I won't be able to diagnose without. Please provide a suitable alternative for.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456887720
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
I don't like this style of code passing thread-unsafe Collections from T1 to T2, etc. Again dangerous but not because of modifications. It is because of memory visibility between the CPU cache and the main memory. The `ArrayList` does not make it!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456902065
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
But i think this hides a real solution because the `while` loop does nothing and the lock per Thread again does nothing because the main purpose of Lock is to be shared across threads.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456886502
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
Please read the API of `SyncContextFactory` first.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661291308
> Syncontext must be global if you want an useful global lock but it uses an int so it is not thread safe. If not global it does not impl its original goal so in all cases it looks incomplete.
I absolutely do not agree with that. If SyncContext would be global one would not need the factory at all. It would be ridiculous. Reread the API requirements on SyncContext instances. The global lock resides in the singleton factory and it is used throughout. The int *is* threadsafe because it lives in the SyncContext instance only, it is not shared. Please provide a PR which depicts your understanding of the truth.
> Do you have particular links? Also the point to enable to lock per artifact is not bound to these impl issues, the impl still proves there is no mem issue to do it
https://github.com/takari/takari-local-repository/issues/12 as well as the MRESOLVER-123, -25 and -114. I requested to use the Takari extension in the spirit that it works. It turned out not to. Read through the discussions, take the time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661375379
acquire() is not. Neither is LockSyncContext. This is obvious from the docs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456887921
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
The collections passed has there for years, I did not invent this and it is also ignored on the default implementation. How can this be a problem? Let's focus solely on the PR and not the outer code because that would be a different discussion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] elharo commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r453216419
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.debug( "Acquiring global {} lock (currently held: {})",
Review comment:
They're helpful for the ~0.001% of cases where someone cares about these messages. They're log junk for everyone else. Our logs are already far too full of random logging statements that no one ever reads, all of which obscure the small percentage of actionable information.
Perhaps we need to send messages the end user should read to a different place than all the random logs. But until we can do that we should be removing log statements, not adding more.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456789040
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
This is really dangerous and antipattern because you here can call lock/unlock in different threads since you have two methods. See the Javadoc. It says to use try-finally. And it is because the method contaxt is always called in one thread and that's the point of locking that you lock and unlock your thread. But unparking another thread is dangerous.
Maybe change the API or the abstraction in this class but do not use these principles.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rmannibucau commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661057656
@michael-o fact is current API is not a SPI (as expected) but a specific impl for a global lock. I think this is wrong and is not what is epxected by such work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661296728
@Tibor17 I thought the same, but that's not true because map is never shared, but scoped to the current thread. So the access is sequential. The `FileLockManager` has to be theadsafe. Put if absent would be necessary, if and only if the map would be in the factory and passed down to instances. Then I would agree with you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457405575
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
Review comment:
This one is also invalid. Read the specs:
> A synchronization context is meant to be utilized by only one thread and as such is not thread-safe.
And this is how it works. Nothing is shared among threads.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-660647245
@eolivelli
Can you please interpret every single line in this change? What they do?
I would really like to see the sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456885982
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
Then we would be able to find the right model.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456885901
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
Every concurrency must be tailor made to certain use cases.
What you wanted to achieve?
Pls talk in the context:
T1 does ...
T2 waits until ...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456833473
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
Without having knowledge about use of SyncConext API and it's usage, I don't quite get: with CountDownLatch, how would you know how many to count down upfront?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661295037
@rmannibucau
i was talking to @michael-o about the Map<File, Lock> but having a look at the takari impl i have to say that they are not totally safe wt the [Line60](https://github.com/takari/takari-local-repository/blob/master/src/main/java/io/takari/aether/concurrency/LockingSyncContext.java#L60) because they rewrite the Map entry concurrently. They have to fix it by [Java 8 Functions and ConcurrentHampMap](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentMap.html#computeIfAbsent-K-java.util.function.Function-).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rmannibucau commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661280048
2. Syncontext must be global if you want an useful global lock but it uses an int so it is not thread safe. If not global it does not impl its original goal so in all cases it looks incomplete.
3. Looked at github bugtracker as you requested and it does not look terrible and not worse that this PR was to me. Do you have particular links? Also the point to enable to lock per artifact is not bound to these impl issues, the impl still proves there is no mem issue to do it (was a point against it which is not justified technically so wanted to ensure we mark this topic as done).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456886206
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
The whole purpose is to lock access to specific file system paths from several threads. We don't want to write multiple threads to one file at a time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457403082
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
Review comment:
This point is invalid because you are questioning the interface definition and the provided implementation. I am just doing what the interface expects me to.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rmannibucau commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661093235
Well there are multiple options:
1. Do nothing (we keep these issues) - don't think it would be good
2. Do a global not thread safe lock (and you fix a few issues but create new issues and slow down builds when enabled)
3. Do a concurrent lock mecanism (as https://github.com/takari/takari-local-repository/blob/master/src/main/java/io/takari/aether/concurrency/LockingSyncContext.java)
I think 2 solves as much as it breaks so I'd like to ensure if it is done that it is compatible with 3. Current impl seems to stack workarounds (lock counter means locking is not cleanly handled in resolver, global locking API vs per artifact or worse case per groupId).
I'd also add that on windows this kind of impl can just lock silently and never end (so can need a timeout?) :(.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661311488
Is the instance of `LockingSyncContext` concurrent?
Is the `acquire` method concurrent?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661059577
@rmannibucau If you are unhappy with the SPI, create a JIRA issue we can discuss for Resolver 2.0. The SPI *does* support any kind of locks. I have chosen to provide the simplest one. What do you expect if the requirement says: "Provide a global locking sync context by default"? I do not really understand what you expect.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457495658
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
@michael-o
Can you explain the concept of clocking critical sections with read and write lock separately? We are downloading and writing a file. Writing a stream on the disk must be treated by the write lock because it is exclusive lock. Where is the read lock used and why? I guess this issue is about locking the writes to the disk from one JVM but two or more threads.
Additionally, the fairness must not be taken as any functionality. No logic must rely on any fairness.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456887762
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
The counter does *not* count threads, it conts the reentrant locks held by the current thread. A SyncContext is never shared. How can this lead to a deadlock if this is never shared?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rmannibucau commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457240942
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
Review comment:
1. Is the session needed there? can't it be injected?
2. Is shared needed there, shouldn't the context handle that since it is the one holding the underlying lock
For me the factory should just load an impl and be useless if there is no pluggability there, context is where the abstraction is (see next comment)
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
Review comment:
this class is not threadsafe but should be otherwise this default impl does not work as soon as you use -T
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
+ lockHoldCount++;
}
public void close()
Review comment:
I would drop it in favor of th returned value of acquire.
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
Review comment:
Think it is better to return a Map of locks (or just Runnable which would be lock::unlock) since for more fine grained impls, some lock can fail whereas others would work. In such case, we should let the caller handle it properly.
Can be:
Map<Object, Runnable>
for the simplest form (or typed if you prefer, just care that functionally it matches fine grained impls).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456789312
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
@michael-o
@elharo
See my comment above as well https://github.com/apache/maven-resolver/pull/65/commits/782659ac9a7e708bc61695c964cf0bf9646f652d#r456789040.
I think all you wanted to use was the `CountDownLatch`.
In case you are wishing to use the Java Concurrency API, pls ping me on the Slack! I have very long experiences with this API.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rmannibucau commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457409869
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
+ lockHoldCount++;
}
public void close()
Review comment:
Since I proposed to not return void but actual locks (to handle errors) then close() would be useless
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456921858
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
Of course it does. Please read the documentation of `ReentrantReadWriteLock`. You have to call unlock as often as you have called lock from the very same thread.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456886644
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
I did but i am trying to explain that you cannot achive properly what you want on this level:
> try {
> syncContext.acquire( artifacts, metadatas );
> // work with the artifacts and metadatas
> } finally {
> syncContext.close();
> }
You have to do it on the caller. This means the method which has this try-filanny code, see above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] elharo commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r453196955
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.debug( "Acquiring global {} lock (currently held: {})",
Review comment:
Please remove these debug logs. They're not actionable and our logs are too big already. Also note, users cannot always set logging levels in jobs; e.g. on our own CI jobs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457649439
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
> We are downloading and writing a file. Writing a stream on the disk must be treated by the write lock because it is exclusive lock
Correct.
> Where is the read lock used and why?
Please look into the code. I am not the caller, but the callee only.
> I guess this issue is about locking the writes to the disk from one JVM but two or more threads.
Correct also.
> Additionally, the fairness must not be taken as any functionality. No logic must rely on any fairness.
I take this as a fair point. This can be addressed in a separate issue/release, but should not make any difference in regard of locking, but order only.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661648420
Access to files is concurrent.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456887401
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
The`nested` method calls `acquire` may reach a deadlock because the `int` is the counter of threads but what if their number of jobs is higher and one is waiting for another - deadlock on thread resources.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456789312
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
@michael-o
@elharo
See my comment above as well https://github.com/apache/maven-resolver/pull/65/commits/782659ac9a7e708bc61695c964cf0bf9646f652d#r456789040.
I think all you wanted was the `CountDownLatch`.
In case you are wishing to use the Java Concurrency API, pls ping me on the Slack! I have very long experiences with this API.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r453230187
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.debug( "Acquiring global {} lock (currently held: {})",
Review comment:
I will move them to trace level. The first and foremost reason is that you do not see which sync context factory is in use thus making sure you are actually using the one you want.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rmannibucau commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457417864
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
+ lockHoldCount++;
}
public void close()
Review comment:
not really, client is the resolver here, it must be a resolver SPI and it can should async friendly (for better concurrent downloads) IMHO. Also note it is not worse than a close method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661050855
@rmannibucau Thanks, looking trough..judging from your comments you have neither read the API contract not the JIRA issues.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457401794
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
Review comment:
This point is invalid because the JIRA issue clearly says that it will provide a global lock. If you want to provide an additional `SyncContextFactory` implementation we can easily add this as a separate module.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rmannibucau commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661669355
If the context is not global it is broken in case of -T and 2 modules resolving concurrently the same artifact - or you synchronize it and break concurrent feature. So at the end it must be - as a map must be for the per artifact lock.
Agree takari impl has the mentionned issue - but comments make me think any file based impl too - but it is less far than a complete impl - in terms of design - than a global lock IMHO (the concurrent impl is key for me since most user project will me multimodules).
So if i summarize: file locking must happen in a single thread per jvm and I would like it to be per artifact.
Means we must have a dedicated thread with an event loop dedicated to lock files ($artifact.lock ?) And releasing them? Wouldnt it work better? Then state per artifact does not need the int or lock there too since the second lock call would block the caller (with a completionstage or equivalent callback like api) but not the event loop itself.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661068773
Currently, there is nothing available: broken, see depending issues. Believe, most people don't even know how to enable something via extensions, they will simply file a bug. The only contention is during write and first resolution, but I have expressed several times in the ticket. What would you explicitly do now to the longstanding concurrency issues given that no one did anything at all, I don't expect anyone to add anything more sophisticated?!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o edited a comment on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661291308
> Syncontext must be global if you want an useful global lock but it uses an int so it is not thread safe. If not global it does not impl its original goal so in all cases it looks incomplete.
I absolutely do not agree with that. If SyncContext would be global one would not need the factory at all. It would be ridiculous. Reread the API requirements on SyncContext instances. The global lock resides in the singleton factory and it is used throughout. The int *is* threadsafe because it lives in the SyncContext instance only, it is not shared. Please provide a PR which depicts your understanding of the truth.
> Do you have particular links? Also the point to enable to lock per artifact is not bound to these impl issues, the impl still proves there is no mem issue to do it
https://github.com/takari/takari-local-repository/issues/12 as well as the MRESOLVER-123, -25 and -114. I requested to use the Takari extension in the spirit that it works. It turned out not to. Read through the discussions, take the time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] rmannibucau commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661063742
@michael-o for me any provided impl must be concurrent friendly since it is a core feature of maven otherwise first bug is that the feature is not compatible wiht maven. A enabl-able global lock is ok for me while default impl is aligned on maven, this is my main concern. A first delivery with less features is acceptable while API enables to support it in a coming release.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457400875
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
+ lockHoldCount++;
}
public void close()
Review comment:
What would yo exactly drop? `acquire()` is `void`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o edited a comment on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661648420
Access to files is concurrent. The concurrency is in the system, at most sync context factory and always in/with the locks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] asfgit closed pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r456789312
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
Review comment:
@michael-o
@elharo
See my comment above as well https://github.com/apache/maven-resolver/pull/65/commits/782659ac9a7e708bc61695c964cf0bf9646f652d#r456789040.
I think all you wanted was the `CountDownLatch`.
In can you are wishing to use Java Concurrency API, pls ping me on the Slack! I have very long experiences with this API.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661272254
Please explain point 2. Why do you consider it as non-thread safe? The lock itself is thread-safe and can be passed around. That's the hole point. The `SyncContext` does not has to be.
3. This one is broken too, mentioned numerous times in tickets and on GitHub.
Why does 2 has to be compatible with 3? It has only to comply with the API contract. There is no current implementation, it is a no-op. Also there is no global locking API in the code whatsoever. The global locking is a design decision for this specific implementation.
> I'd also add that on windows this kind of impl can just lock silently and never end (so can need a timeout?) :(.
Please explain why this is a problem on Windows.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#discussion_r457412151
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultSyncContextFactory.java
##########
@@ -28,31 +30,59 @@
import org.eclipse.aether.artifact.Artifact;
import org.eclipse.aether.impl.SyncContextFactory;
import org.eclipse.aether.metadata.Metadata;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.inject.Singleton;
/**
- * A factory to create synchronization contexts. This default implementation actually does not provide any real
- * synchronization but merely completes the repository system.
+ * A factory to create synchronization contexts. This default implementation uses fair global locking
+ * based on {@link ReentrantReadWriteLock}. Explicit artifacts and metadata passed are ignored.
*/
@Named
+@Singleton
public class DefaultSyncContextFactory
implements SyncContextFactory
{
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock( true );
public SyncContext newInstance( RepositorySystemSession session, boolean shared )
{
- return new DefaultSyncContext();
+ return new DefaultSyncContext( shared ? lock.readLock() : lock.writeLock(), shared );
}
static class DefaultSyncContext
implements SyncContext
{
+ private static final Logger LOGGER = LoggerFactory.getLogger( DefaultSyncContext.class );
+
+ private final Lock lock;
+ private final boolean shared;
+ private int lockHoldCount;
+
+ DefaultSyncContext( Lock lock, boolean shared )
+ {
+ this.lock = lock;
+ this.shared = shared;
+ }
public void acquire( Collection<? extends Artifact> artifact, Collection<? extends Metadata> metadata )
{
+ LOGGER.trace( "Acquiring global {} lock (currently held: {})",
+ shared ? "read" : "write", lockHoldCount );
+ lock.lock();
+ lockHoldCount++;
}
public void close()
Review comment:
I cannot change the interface, even if the try-with-resources wouldn't be usable anymore. You would move the task to the client to fiddle with lowlevel locks. This makes no sense. Especially because an implementation is not forced to implement the `Lock` interface.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] Tibor17 edited a comment on pull request #65: [MRESOLVER-123] Provide a global locking sync context by default
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #65:
URL: https://github.com/apache/maven-resolver/pull/65#issuecomment-661295037
@rmannibucau
i was talking to @michael-o about the `Map<File, Lock>` , but having a look at the takari impl i have to say that they are not totally safe wit the [Line60](https://github.com/takari/takari-local-repository/blob/master/src/main/java/io/takari/aether/concurrency/LockingSyncContext.java#L60) because they rewrite the Map entry concurrently. They have to fix it by [Java 8 Functions and ConcurrentHampMap](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentMap.html#computeIfAbsent-K-java.util.function.Function-).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org