gnodet commented on code in PR #1814: URL: https://github.com/apache/maven-resolver/pull/1814#discussion_r2999389753
########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/NamedLocksTrackingFileManager.java: ########## @@ -0,0 +1,201 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.internal.impl; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.TimeUnit; + +import org.eclipse.aether.named.NamedLock; +import org.eclipse.aether.named.NamedLockFactory; +import org.eclipse.aether.named.NamedLockKey; +import org.eclipse.aether.util.StringDigestUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Manages access to a properties file using named locks. + * <p> + * This implementation uses {@link NamedLock} to protect tracking files from concurrent access, and can be used + * when it is single "modern" Maven version used to access same local repository concurrently. + * + * @since 2.0.17 + * @see LegacyTrackingFileManager + * @see TrackingFileManagerProvider + */ +public final class NamedLocksTrackingFileManager implements TrackingFileManager { + private static final Logger LOGGER = LoggerFactory.getLogger(NamedLocksTrackingFileManager.class); + + private final NamedLockFactory namedLockFactory; + private final long time; + private final TimeUnit unit; + + public NamedLocksTrackingFileManager(NamedLockFactory namedLockFactory, long time, TimeUnit unit) { + this.namedLockFactory = namedLockFactory; + this.time = time; + this.unit = unit; + } + + @Deprecated + @Override + public Properties read(File file) { + return read(file.toPath()); + } + + @Override + public Properties read(Path path) { + if (Files.isReadable(path)) { + try (NamedLock namedLock = namedLock(path)) { + if (namedLock.lockShared(time, unit)) { + try { + Properties props = new Properties(); + try (InputStream in = Files.newInputStream(path)) { + props.load(in); + } + return props; + } catch (NoSuchFileException e) { + LOGGER.debug("No such file to read {}: {}", path, e.getMessage()); + } catch (IOException e) { + LOGGER.warn("Failed to read tracking file '{}'", path, e); + throw new UncheckedIOException(e); + } finally { + namedLock.unlock(); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while reading tracking file " + path, e); + } + } + return null; Review Comment: Bug: If `lockShared()` returns `false` (timeout), execution silently falls through to `return null`. Per the `TrackingFileManager` contract, `null` means "file does not exist" — but here the file **does** exist; we just couldn't acquire the lock in time. Callers (e.g., `DefaultUpdateCheckManager`) cannot distinguish between a missing file and a lock timeout, which could lead to stale metadata or redundant downloads. By contrast, `update()` correctly throws `IllegalStateException` on lock timeout (line 143). `read()` and `delete()` should do the same for consistency. At minimum, a WARN-level log would make the situation observable. ```suggestion } } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new IllegalStateException("Interrupted while reading tracking file " + path, e); } } throw new IllegalStateException("Failed to lock for read the tracking file " + path); ``` (Same concern applies to `delete()` below.) ########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/TrackingFileManagerProvider.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.internal.impl; + +import javax.inject.Inject; +import javax.inject.Named; +import javax.inject.Provider; +import javax.inject.Singleton; + +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + +import org.eclipse.aether.ConfigurationProperties; +import org.eclipse.aether.impl.NamedLockFactorySelector; +import org.eclipse.aether.named.NamedLockFactory; +import org.eclipse.aether.util.ConfigUtils; + +/** + * Provides selected instance of {@link TrackingFileManager} implementation. + * + * @since 2.0.17 + */ +@Singleton +@Named +public class TrackingFileManagerProvider implements Provider<TrackingFileManager> { + public static final String CONFIG_PROPS_PREFIX = ConfigurationProperties.PREFIX_SYSTEM + "trackingFileManager."; + + /** + * Name of the tracking file manager to use. Supported values are "namedLocks" and "legacy". The latter should be + * used if it is known, that local repository is simultaneously accessed by Maven 3.10+ and older Maven versions. + * This decision happens early, during boot of the system, hence system properties can be used only as configuration + * source. + * + * @configurationSource {@link System#getProperty(String, String)} + * @configurationType {@link java.lang.String} + * @configurationDefaultValue {@link #DEFAULT_TRACKING_FILE_MANAGER_NAME} + */ + public static final String CONFIG_PROP_TRACKING_FILE_MANAGER_NAME = CONFIG_PROPS_PREFIX + "name"; + + public static final String DEFAULT_TRACKING_FILE_MANAGER_NAME = "legacy"; + + private final TrackingFileManager trackingFileManager; + + /** + * Default constructor, to be used in tests; provides "legacy" tracking file manager only. + */ + public TrackingFileManagerProvider() { + this.trackingFileManager = new LegacyTrackingFileManager(); + } + + /** + * Constructor to be used in production. + */ + @Inject + public TrackingFileManagerProvider(NamedLockFactorySelector selector) { + // this is early construction; no session, hence we must rely on system properties instead + Map<String, String> config = System.getProperties().entrySet().stream() + .collect(Collectors.toMap( + e -> String.valueOf(e.getKey()), + e -> String.valueOf(e.getValue()), + (prev, next) -> next, + HashMap::new)); Review Comment: Minor: `System.getProperties()` returns the **live** `Properties` hashtable. Streaming its `entrySet()` while another thread calls `System.setProperty()` could throw `ConcurrentModificationException`. Unlikely during early boot, but could be made robust, e.g.: ```suggestion Map<String, String> config = new HashMap<>(); for (String name : System.getProperties().stringPropertyNames()) { config.put(name, System.getProperty(name)); } ``` `stringPropertyNames()` returns a snapshot of the keys, making iteration safe. ########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/named/DefaultNamedLockFactorySelector.java: ########## @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.internal.impl.named; + +import javax.inject.Inject; +import javax.inject.Named; +import javax.inject.Singleton; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.TimeUnit; + +import org.eclipse.aether.ConfigurationProperties; +import org.eclipse.aether.MultiRuntimeException; +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.impl.NamedLockFactorySelector; +import org.eclipse.aether.impl.RepositorySystemLifecycle; +import org.eclipse.aether.internal.impl.synccontext.named.NamedLockFactoryAdapter; +import org.eclipse.aether.internal.impl.synccontext.named.NamedLockFactoryAdapterFactoryImpl; +import org.eclipse.aether.named.NamedLockFactory; +import org.eclipse.aether.named.providers.FileLockNamedLockFactory; +import org.eclipse.aether.util.ConfigUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static java.util.Objects.requireNonNull; + +@Singleton +@Named +public class DefaultNamedLockFactorySelector implements NamedLockFactorySelector { + public static final String CONFIG_PROPS_PREFIX = ConfigurationProperties.PREFIX_SYSTEM + "named."; + + /** + * Name of the lock factory to use in system. Out of the box supported ones are "file-lock", "rwlock-local", + * "semaphore-local", "noop". By adding extensions one can extend available lock factories (for example IPC locking). + * + * @configurationSource {@link RepositorySystemSession#getConfigProperties()} + * @configurationType {@link java.lang.String} + * @configurationDefaultValue {@link #DEFAULT_FACTORY_NAME} + */ + public static final String CONFIG_PROP_FACTORY_NAME = CONFIG_PROPS_PREFIX + "factory"; + + public static final String DEFAULT_FACTORY_NAME = FileLockNamedLockFactory.NAME; + + /** + * The maximum amount of time to be blocked, while obtaining a named lock. + * + * @configurationSource {@link RepositorySystemSession#getConfigProperties()} + * @configurationType {@link java.lang.Long} + * @configurationDefaultValue {@link #DEFAULT_LOCK_WAIT_TIME} + */ + public static final String CONFIG_PROP_LOCK_WAIT_TIME = CONFIG_PROPS_PREFIX + "time"; + + public static final long DEFAULT_LOCK_WAIT_TIME = 900L; + + /** + * The unit of maximum amount of time. Accepts TimeUnit enum names. + * + * @configurationSource {@link RepositorySystemSession#getConfigProperties()} + * @configurationType {@link java.lang.String} + * @configurationDefaultValue {@link #DEFAULT_LOCK_WAIT_TIME_UNIT} + */ + public static final String CONFIG_PROP_LOCK_WAIT_TIME_UNIT = CONFIG_PROPS_PREFIX + "timeUnit"; + + public static final String DEFAULT_LOCK_WAIT_TIME_UNIT = "SECONDS"; + + protected final Logger logger = LoggerFactory.getLogger(getClass()); + + protected final Map<String, NamedLockFactory> factories; + + protected final ConcurrentMap<String, NamedLockFactory> usedFactories; + + @Inject + public DefaultNamedLockFactorySelector( + Map<String, NamedLockFactory> factories, RepositorySystemLifecycle lifecycle) { + this.factories = requireNonNull(factories); + this.usedFactories = new ConcurrentHashMap<>(); + lifecycle.addOnSystemEndedHandler(this::shutdown); + + logger.debug("Created lock factory selector; available lock factories {}", factories.keySet()); + } + + @Override + public Set<String> getAvailableLockFactories() { + return Collections.unmodifiableSet(new HashSet<>(factories.keySet())); + } + + @Override + public NamedLockFactory getNamedLockFactory(Map<String, ?> configuration) { + String factoryName = ConfigUtils.getString( + configuration, + DEFAULT_FACTORY_NAME, + CONFIG_PROP_FACTORY_NAME, + NamedLockFactoryAdapterFactoryImpl.CONFIG_PROP_FACTORY_KEY); + NamedLockFactory factory = factories.get(factoryName); + if (factory == null) { + throw new IllegalArgumentException( + "Unknown NamedLockFactory name: '" + factoryName + "', known ones: " + factories.keySet()); + } + usedFactories.putIfAbsent(factoryName, factory); + return factory; + } + + @Override + public long getLockWaitTime(Map<String, ?> configuration) { + long result = ConfigUtils.getLong( + configuration, + DEFAULT_LOCK_WAIT_TIME, + CONFIG_PROP_LOCK_WAIT_TIME, + NamedLockFactoryAdapter.CONFIG_PROP_TIME); + if (result <= 0) { + throw new IllegalArgumentException( + "The " + CONFIG_PROP_LOCK_WAIT_TIME_UNIT + " configuration but be grater than zero"); Review Comment: Bug: Two issues here — "but be grater" should be "must be greater", and the referenced config key should be `CONFIG_PROP_LOCK_WAIT_TIME` (not `_UNIT`), since this validates the **time** value, not the unit. ```suggestion "The " + CONFIG_PROP_LOCK_WAIT_TIME + " configuration must be greater than zero"); ``` ########## maven-resolver-impl/src/main/java/org/eclipse/aether/impl/NamedLockFactorySelector.java: ########## @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.impl; + +import java.util.Map; +import java.util.Set; +import java.util.concurrent.TimeUnit; + +import org.eclipse.aether.named.NamedLockFactory; + +/** + * Internal selector helping to for system-wide use of named locks. Review Comment: Nit: broken grammar. ```suggestion * Selector for system-wide use of named locks. ``` ########## maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/NamedLocksTrackingFileManagerTest.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.internal.impl; + +import java.util.concurrent.TimeUnit; + +import org.eclipse.aether.named.providers.LocalReadWriteLockNamedLockFactory; + +public class NamedLocksTrackingFileManagerTest extends TrackingFileManagerTestSupport { + @Override + protected TrackingFileManager createTrackingFileManager(FS fs) { + return new NamedLocksTrackingFileManager(new LocalReadWriteLockNamedLockFactory(), 1L, TimeUnit.SECONDS); Review Comment: This test uses `LocalReadWriteLockNamedLockFactory` (JVM-local `ReentrantReadWriteLock`). Since the production default is `FileLockNamedLockFactory` (`file-lock`), the inter-process locking path is untested. Consider adding a parallel test class (or parameterization) with `FileLockNamedLockFactory` for the `DEFAULT` filesystem case. ########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/NamedLocksTrackingFileManager.java: ########## @@ -0,0 +1,201 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.internal.impl; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.TimeUnit; + +import org.eclipse.aether.named.NamedLock; +import org.eclipse.aether.named.NamedLockFactory; +import org.eclipse.aether.named.NamedLockKey; +import org.eclipse.aether.util.StringDigestUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Manages access to a properties file using named locks. + * <p> + * This implementation uses {@link NamedLock} to protect tracking files from concurrent access, and can be used + * when it is single "modern" Maven version used to access same local repository concurrently. + * + * @since 2.0.17 + * @see LegacyTrackingFileManager + * @see TrackingFileManagerProvider + */ +public final class NamedLocksTrackingFileManager implements TrackingFileManager { + private static final Logger LOGGER = LoggerFactory.getLogger(NamedLocksTrackingFileManager.class); + + private final NamedLockFactory namedLockFactory; + private final long time; + private final TimeUnit unit; + + public NamedLocksTrackingFileManager(NamedLockFactory namedLockFactory, long time, TimeUnit unit) { + this.namedLockFactory = namedLockFactory; + this.time = time; + this.unit = unit; + } + + @Deprecated + @Override + public Properties read(File file) { + return read(file.toPath()); + } + + @Override + public Properties read(Path path) { + if (Files.isReadable(path)) { + try (NamedLock namedLock = namedLock(path)) { + if (namedLock.lockShared(time, unit)) { + try { + Properties props = new Properties(); + try (InputStream in = Files.newInputStream(path)) { + props.load(in); + } + return props; + } catch (NoSuchFileException e) { + LOGGER.debug("No such file to read {}: {}", path, e.getMessage()); + } catch (IOException e) { + LOGGER.warn("Failed to read tracking file '{}'", path, e); + throw new UncheckedIOException(e); + } finally { + namedLock.unlock(); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while reading tracking file " + path, e); + } + } + return null; + } + + @Deprecated + @Override + public Properties update(File file, Map<String, String> updates) { + return update(file.toPath(), updates); + } + + @Override + public Properties update(Path path, Map<String, String> updates) { + try { + Files.createDirectories(path.getParent()); + } catch (IOException e) { + LOGGER.warn("Failed to create tracking file parent '{}'", path, e); + throw new UncheckedIOException(e); + } + try (NamedLock lock = namedLock(path)) { + if (lock.lockExclusively(time, unit)) { + try { + Properties props = new Properties(); + if (Files.isRegularFile(path)) { + try (InputStream stream = Files.newInputStream(path, StandardOpenOption.READ)) { + props.load(stream); + } + } + for (Map.Entry<String, String> update : updates.entrySet()) { + if (update.getValue() == null) { + props.remove(update.getKey()); + } else { + props.setProperty(update.getKey(), update.getValue()); + } + } + LOGGER.debug("Writing tracking file '{}'", path); + try (OutputStream out = Files.newOutputStream(path)) { + props.store( + out, + "NOTE: This is a Maven Resolver internal implementation file" + + ", its format can be changed without prior notice."); + } + return props; + } catch (IOException e) { + LOGGER.warn("Failed to write tracking file '{}'", path, e); + throw new UncheckedIOException(e); + } finally { + lock.unlock(); + } + } + throw new IllegalStateException("Failed to lock for update the tracking file " + path); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while updating tracking file " + path, e); + } + } + + @Deprecated + @Override + public boolean delete(File file) { + return delete(file.toPath()); + } + + @Override + public boolean delete(Path path) { + if (Files.isReadable(path)) { + try (NamedLock lock = namedLock(path)) { + if (lock.lockExclusively(time, unit)) { + try { + Files.delete(path); + return true; + } catch (NoSuchFileException e) { + LOGGER.debug("No such file to delete {}: {}", path, e.getMessage()); + } catch (IOException e) { + LOGGER.warn("Failed to delete tracking file '{}'", path, e); + throw new UncheckedIOException(e); + } finally { + lock.unlock(); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while deleting tracking file " + path, e); + } + } + return false; Review Comment: Same issue as `read()`: if `lockExclusively()` returns `false` (timeout), `delete()` silently returns `false`, which per contract means "file didn't exist." This is misleading — the file exists but couldn't be locked. Should throw like `update()` does. ```suggestion } } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new IllegalStateException("Interrupted while deleting tracking file " + path, e); } } throw new IllegalStateException("Failed to lock for delete the tracking file " + path); ``` ########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NamedLockFactoryAdapter.java: ########## @@ -48,28 +48,28 @@ public final class NamedLockFactoryAdapter { /** * The maximum of time amount to be blocked to obtain lock. + * <strong>Deprecated: use {@code aether.system.named...} configuration instead.</strong> * * @since 1.7.0 * @configurationSource {@link RepositorySystemSession#getConfigProperties()} * @configurationType {@link java.lang.Long} - * @configurationDefaultValue {@link #DEFAULT_TIME} + * @deprecated */ + @Deprecated public static final String CONFIG_PROP_TIME = CONFIG_PROPS_PREFIX + "time"; Review Comment: Note: `DEFAULT_TIME` and `DEFAULT_TIME_UNIT` (previously at this location) were `public static final` constants. Removing them is a binary-incompatible change for any downstream code that referenced `NamedLockFactoryAdapter.DEFAULT_TIME` or `NamedLockFactoryAdapter.DEFAULT_TIME_UNIT`. The config property *strings* (`CONFIG_PROP_TIME`, `CONFIG_PROP_TIME_UNIT`) are correctly deprecated rather than removed. Consider giving the same treatment to the default-value constants — deprecate and keep them, pointing to `DefaultNamedLockFactorySelector.DEFAULT_LOCK_WAIT_TIME` etc. ########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/named/DefaultNamedLockFactorySelector.java: ########## @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.internal.impl.named; + +import javax.inject.Inject; +import javax.inject.Named; +import javax.inject.Singleton; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.TimeUnit; + +import org.eclipse.aether.ConfigurationProperties; +import org.eclipse.aether.MultiRuntimeException; +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.impl.NamedLockFactorySelector; +import org.eclipse.aether.impl.RepositorySystemLifecycle; +import org.eclipse.aether.internal.impl.synccontext.named.NamedLockFactoryAdapter; +import org.eclipse.aether.internal.impl.synccontext.named.NamedLockFactoryAdapterFactoryImpl; +import org.eclipse.aether.named.NamedLockFactory; +import org.eclipse.aether.named.providers.FileLockNamedLockFactory; +import org.eclipse.aether.util.ConfigUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static java.util.Objects.requireNonNull; + +@Singleton +@Named +public class DefaultNamedLockFactorySelector implements NamedLockFactorySelector { + public static final String CONFIG_PROPS_PREFIX = ConfigurationProperties.PREFIX_SYSTEM + "named."; + + /** + * Name of the lock factory to use in system. Out of the box supported ones are "file-lock", "rwlock-local", + * "semaphore-local", "noop". By adding extensions one can extend available lock factories (for example IPC locking). + * + * @configurationSource {@link RepositorySystemSession#getConfigProperties()} + * @configurationType {@link java.lang.String} + * @configurationDefaultValue {@link #DEFAULT_FACTORY_NAME} + */ + public static final String CONFIG_PROP_FACTORY_NAME = CONFIG_PROPS_PREFIX + "factory"; + + public static final String DEFAULT_FACTORY_NAME = FileLockNamedLockFactory.NAME; + + /** + * The maximum amount of time to be blocked, while obtaining a named lock. + * + * @configurationSource {@link RepositorySystemSession#getConfigProperties()} + * @configurationType {@link java.lang.Long} + * @configurationDefaultValue {@link #DEFAULT_LOCK_WAIT_TIME} + */ + public static final String CONFIG_PROP_LOCK_WAIT_TIME = CONFIG_PROPS_PREFIX + "time"; + + public static final long DEFAULT_LOCK_WAIT_TIME = 900L; + + /** + * The unit of maximum amount of time. Accepts TimeUnit enum names. + * + * @configurationSource {@link RepositorySystemSession#getConfigProperties()} + * @configurationType {@link java.lang.String} + * @configurationDefaultValue {@link #DEFAULT_LOCK_WAIT_TIME_UNIT} + */ + public static final String CONFIG_PROP_LOCK_WAIT_TIME_UNIT = CONFIG_PROPS_PREFIX + "timeUnit"; Review Comment: Nit: naming inconsistency. The existing (now deprecated) key uses dot-separated `time.unit` (`aether.syncContext.named.time.unit`), and the companion `time` key here also uses dots (`aether.system.named.time`). But this new key uses camelCase: `aether.system.named.timeUnit`. Consider `time.unit` for consistency: ```suggestion public static final String CONFIG_PROP_LOCK_WAIT_TIME_UNIT = CONFIG_PROPS_PREFIX + "time.unit"; ``` ########## maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/NamedLocksTrackingFileManager.java: ########## @@ -0,0 +1,201 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.internal.impl; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.TimeUnit; + +import org.eclipse.aether.named.NamedLock; +import org.eclipse.aether.named.NamedLockFactory; +import org.eclipse.aether.named.NamedLockKey; +import org.eclipse.aether.util.StringDigestUtil; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Manages access to a properties file using named locks. + * <p> + * This implementation uses {@link NamedLock} to protect tracking files from concurrent access, and can be used + * when it is single "modern" Maven version used to access same local repository concurrently. + * + * @since 2.0.17 + * @see LegacyTrackingFileManager + * @see TrackingFileManagerProvider + */ +public final class NamedLocksTrackingFileManager implements TrackingFileManager { + private static final Logger LOGGER = LoggerFactory.getLogger(NamedLocksTrackingFileManager.class); + + private final NamedLockFactory namedLockFactory; + private final long time; + private final TimeUnit unit; + + public NamedLocksTrackingFileManager(NamedLockFactory namedLockFactory, long time, TimeUnit unit) { + this.namedLockFactory = namedLockFactory; + this.time = time; + this.unit = unit; + } + + @Deprecated + @Override + public Properties read(File file) { + return read(file.toPath()); + } + + @Override + public Properties read(Path path) { + if (Files.isReadable(path)) { + try (NamedLock namedLock = namedLock(path)) { + if (namedLock.lockShared(time, unit)) { + try { + Properties props = new Properties(); + try (InputStream in = Files.newInputStream(path)) { + props.load(in); + } + return props; + } catch (NoSuchFileException e) { + LOGGER.debug("No such file to read {}: {}", path, e.getMessage()); + } catch (IOException e) { + LOGGER.warn("Failed to read tracking file '{}'", path, e); + throw new UncheckedIOException(e); + } finally { + namedLock.unlock(); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while reading tracking file " + path, e); + } + } + return null; + } + + @Deprecated + @Override + public Properties update(File file, Map<String, String> updates) { + return update(file.toPath(), updates); + } + + @Override + public Properties update(Path path, Map<String, String> updates) { + try { + Files.createDirectories(path.getParent()); + } catch (IOException e) { + LOGGER.warn("Failed to create tracking file parent '{}'", path, e); + throw new UncheckedIOException(e); + } + try (NamedLock lock = namedLock(path)) { + if (lock.lockExclusively(time, unit)) { + try { + Properties props = new Properties(); + if (Files.isRegularFile(path)) { + try (InputStream stream = Files.newInputStream(path, StandardOpenOption.READ)) { + props.load(stream); + } + } + for (Map.Entry<String, String> update : updates.entrySet()) { + if (update.getValue() == null) { + props.remove(update.getKey()); + } else { + props.setProperty(update.getKey(), update.getValue()); + } + } + LOGGER.debug("Writing tracking file '{}'", path); + try (OutputStream out = Files.newOutputStream(path)) { + props.store( + out, + "NOTE: This is a Maven Resolver internal implementation file" + + ", its format can be changed without prior notice."); + } + return props; + } catch (IOException e) { + LOGGER.warn("Failed to write tracking file '{}'", path, e); + throw new UncheckedIOException(e); + } finally { + lock.unlock(); + } + } + throw new IllegalStateException("Failed to lock for update the tracking file " + path); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while updating tracking file " + path, e); + } + } + + @Deprecated + @Override + public boolean delete(File file) { + return delete(file.toPath()); + } + + @Override + public boolean delete(Path path) { + if (Files.isReadable(path)) { + try (NamedLock lock = namedLock(path)) { + if (lock.lockExclusively(time, unit)) { + try { + Files.delete(path); + return true; + } catch (NoSuchFileException e) { + LOGGER.debug("No such file to delete {}: {}", path, e.getMessage()); + } catch (IOException e) { + LOGGER.warn("Failed to delete tracking file '{}'", path, e); + throw new UncheckedIOException(e); + } finally { + lock.unlock(); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted while deleting tracking file " + path, e); + } + } + return false; + } + + /** + * Creates unique named lock for given path with name that is unique for paths, but carries some extra + * information useful for debugging. + */ + private NamedLock namedLock(Path path) { + return namedLockFactory.getLock(NamedLockKey.of( + "tracking-" + StringDigestUtil.sha1(canonicalPath(path).toString()), path.toString())); + } + + /** + * Tries the best it can to figure out actual file the workload is about, while resolving cases like symlinked + * local repository etc. + */ + private static Path canonicalPath(Path path) { + try { + return path.toRealPath(); + } catch (IOException e) { + return canonicalPath(path.getParent()).resolve(path.getFileName()); Review Comment: Minor: if `path.getParent()` returns `null` (root path or single-component relative path), the recursive call would NPE. In practice tracking files are always under the local repository so this can't happen, but a defensive guard would make this robust: ```suggestion return path.getParent() != null ? canonicalPath(path.getParent()).resolve(path.getFileName()) : path.toAbsolutePath(); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
