Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions p2-maven-plugin/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,16 @@
<groupId>commons-net</groupId>
<artifactId>commons-net</artifactId>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
<build>
<plugins>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*******************************************************************************
* Copyright (c) 2026 Lars Vogel and others.
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
* which accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*******************************************************************************/
package org.eclipse.tycho.p2maven.transport;

/**
* Describes the outcome of a {@link HttpCache} access, so that callers can
* produce accurate log output (e.g. distinguish "Downloading" from "Fetched
* from cache").
*/
public enum CacheState {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please think about better naming here that is independent from caching and agnostic from the transport technique. e.g. we also have ftp transports that have no meaning of (http) status codes.

/** The file was served from the local cache without any network access. */
FROM_CACHE,
/** A conditional request was performed and the server answered 304 Not Modified. */
NOT_MODIFIED,
/** The file was downloaded (full 2xx response with body). */
DOWNLOADED,
/** No information available (e.g. non-HTTP transport or not yet accessed). */
UNKNOWN
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,23 @@ public interface HttpCache {

/**
* Fetches the cache entry for this URI
*
*
* @param uri
* @return
* @throws FileNotFoundException
* if the URI is know to be not found
*/
CacheEntry getCacheEntry(URI uri, Logger logger) throws FileNotFoundException;

/**
* Returns the {@link CacheState} of the most recent access to {@code uri},
* i.e. whether the last {@code getCacheFile} / {@code getLastModified} call
* served the file from the cache, only revalidated it, or actually
* downloaded it. Returns {@link CacheState#UNKNOWN} if the URI has not been
* accessed through this cache.
*/
default CacheState getLastCacheState(URI uri) {
return CacheState.UNKNOWN;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import java.util.Map;
import java.util.Properties;
import java.util.TimeZone;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -62,6 +64,8 @@ public class SharedHttpCacheStorage implements HttpCache {

private final Map<File, CacheLine> entryCache;

private final ConcurrentMap<URI, CacheState> lastCacheState = new ConcurrentHashMap<>();

public SharedHttpCacheStorage() {

entryCache = new LinkedHashMap<>(MAX_CACHE_LINES, 0.75f, true) {
Expand Down Expand Up @@ -122,19 +126,27 @@ public long getLastModified(HttpTransportFactory transportFactory) throws IOExce
@Override
public File getCacheFile(HttpTransportFactory transportFactory) throws IOException {
if (cacheConfig.isOffline()) {
return cacheLine.getFile(normalized, transportFactory, SharedHttpCacheStorage::mavenIsOffline,
logger);
File offlineFile = cacheLine.getFile(normalized, transportFactory,
SharedHttpCacheStorage::mavenIsOffline, logger);
lastCacheState.put(normalized, CacheState.FROM_CACHE);
if (cacheConfig.isInteractive()) {
logger.debug("Fetched from cache: " + normalized);
}
return offlineFile;
}
try {
return cacheLine.fetchFile(normalized, transportFactory, logger);
return cacheLine.fetchFile(normalized, transportFactory, logger,
state -> lastCacheState.put(normalized, state));
} catch (FileNotFoundException | AuthenticationFailedException e) {
// for not found and failed authentication we can't do anything useful
throw e;
} catch (IOException e) {
if (!cacheConfig.isUpdate() && cacheLine.getResponseCode() > 0) {
// if we have something cached, use that ...
logger.warn("Request to " + normalized + " failed, trying cache instead");
return cacheLine.getFile(normalized, transportFactory, nil -> e, logger);
File fallback = cacheLine.getFile(normalized, transportFactory, nil -> e, logger);
lastCacheState.put(normalized, CacheState.FROM_CACHE);
return fallback;
}
throw e;
}
Expand All @@ -143,6 +155,12 @@ public File getCacheFile(HttpTransportFactory transportFactory) throws IOExcepti
};
}

@Override
public CacheState getLastCacheState(URI uri) {
CacheState state = lastCacheState.get(uri.normalize());
return state != null ? state : CacheState.UNKNOWN;
}

private synchronized CacheLine getCacheLine(URI uri) {
String cleanPath = uri.normalize().toASCIIString().replace(':', '/').replace('?', '/').replace('&', '/')
.replace('*', '/').replaceAll("/+", "/");
Expand Down Expand Up @@ -227,8 +245,18 @@ public synchronized long getLastModified(URI uri, HttpTransportFactory transport

public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactory, Logger logger)
throws IOException {
return fetchFile(uri, transportFactory, logger, s -> {
});
}

public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactory, Logger logger,
java.util.function.Consumer<CacheState> stateSink) throws IOException {
boolean exists = file.isFile();
if (exists && !mustValidate()) {
stateSink.accept(CacheState.FROM_CACHE);
if (cacheConfig.isInteractive()) {
logger.debug("Fetched from cache: " + uri);
}
return file;
}
HttpTransport transport = transportFactory.createTransport(uri);
Expand All @@ -248,6 +276,10 @@ public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactor
int code = response.statusCode();
if (exists && code == HttpURLConnection.HTTP_NOT_MODIFIED) {
updateHeader(response, getResponseCode());
stateSink.accept(CacheState.NOT_MODIFIED);
if (cacheConfig.isInteractive()) {
logger.debug("Up-to-date: " + uri);
}
return file;
}
if (isAuthFailure(code)) {
Expand All @@ -264,6 +296,7 @@ public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactor
// Don't save temporary redirects since they might change later, rendering the
// cache entry useless. Save them in the original request URI instead.
transferTemporaryRedirect(transportFactory, uri, redirect, logger);
stateSink.accept(CacheState.DOWNLOADED);
return file;
} else {
File cachedFile = SharedHttpCacheStorage.this.getCacheEntry(redirect, logger)
Expand All @@ -276,13 +309,18 @@ public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactor
// may be returned directly without copying.
response.close(); // early close before doing unrelated file I/O
FileUtils.copyFile(cachedFile, file);
// State for the original URI mirrors the redirect target.
stateSink.accept(SharedHttpCacheStorage.this.getLastCacheState(redirect));
return file;
}
}
if (exists) {
FileUtils.forceDelete(file);
}
response.checkResponseCode();
if (cacheConfig.isInteractive()) {
logger.info("Downloading from " + uri);
}
tempFile = File.createTempFile("download", ".tmp", file.getParentFile());
try (OutputStream os = new BufferedOutputStream(new FileOutputStream(tempFile))) {
response.transferTo(os);
Expand All @@ -292,6 +330,7 @@ public synchronized File fetchFile(URI uri, HttpTransportFactory transportFactor
}
response.close(); // early close before doing file I/O
FileUtils.moveFile(tempFile, file);
stateSink.accept(CacheState.DOWNLOADED);
return file;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ public Thread newThread(Runnable r) {
@Requirement
TransportCacheConfig cacheConfig;

@Requirement
HttpCache httpCache;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should not bind directly to a specific cache implementation there might be other caching techniques see comment below.


@Requirement(role = TransportProtocolHandler.class)
Map<String, TransportProtocolHandler> transportProtocolHandlers;

Expand Down Expand Up @@ -108,18 +111,18 @@ public IStatus downloadArtifact(URI source, OutputStream target, IArtifactDescri
}
}
String id = "p2"; // TODO we might compute the id from the IRepositoryIdManager based on the URI?
if (cacheConfig.isInteractive()) {
logger.info("Downloading from " + id + ": " + source);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This log message should remain to stay consistent with how maven works and to know that the download has started.

try {
DownloadStatusOutputStream statusOutputStream = new DownloadStatusOutputStream(target,
"Download of " + source);
stream(source, monitor).transferTo(statusOutputStream);
DownloadStatus downloadStatus = statusOutputStream.getStatus();
if (cacheConfig.isInteractive()) {
logger.info("Downloaded from " + id + ": " + source + " ("
+ FileUtils.byteCountToDisplaySize(downloadStatus.getFileSize()) + " at "
+ FileUtils.byteCountToDisplaySize(downloadStatus.getTransferRate()) + "/s)");
CacheState state = httpCache.getLastCacheState(source);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of using a side-car approach here, the information should be transported through the DownloadStatusOutputStream

if (state == CacheState.DOWNLOADED || state == CacheState.UNKNOWN) {
logger.info("Downloaded from " + id + ": " + source + " ("
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The info should always be logged and we should just replace the transfer rate by a status e.g.

Downloaded from " + id + ": " + source + " ("
+ FileUtils.byteCountToDisplaySize(downloadStatus.getFileSize()) + " (from cache)"

+ FileUtils.byteCountToDisplaySize(downloadStatus.getFileSize()) + " at "
+ FileUtils.byteCountToDisplaySize(downloadStatus.getTransferRate()) + "/s)");
}
}
return reportStatus(downloadStatus, target);
} catch (AuthenticationFailedException e) {
Expand Down
Loading
Loading