Reissue request when cached tempfile is missing (#11689)

* reissue request if cache file is deleted

* docs

* fmt
This commit is contained in:
Gregory Michael Travis 2024-11-28 03:37:17 -05:00 committed by GitHub
parent 88ba6fa8b8
commit 99928b8ab8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 106 additions and 42 deletions

View File

@ -83,22 +83,36 @@ public class LRUCache<M> {
this.diskSpaceGetter = diskSpaceGetter;
}
/**
* IOExceptions thrown by the HTTP request are propagated; exceptions thrown while creating or
* accessing cache files are caught, and the request is re-issued without caching.
*/
public CacheResult<M> getResult(ItemBuilder<M> itemBuilder)
throws IOException, InterruptedException, ResponseTooLargeException {
String cacheKey = itemBuilder.makeCacheKey();
if (cache.containsKey(cacheKey)) {
return getResultForCacheEntry(cacheKey);
} else {
return makeRequestAndCache(cacheKey, itemBuilder);
try {
if (cache.containsKey(cacheKey)) {
return getResultForCacheEntry(cacheKey);
} else {
return makeRequestAndCache(cacheKey, itemBuilder);
}
} catch (LRUCacheException e) {
// Re-issue the request without caching.
// We don't re-attempt to store the cache file and entry. In some cases
// (such as a cache file deleted from the outside), we could, but this is
// a rare case so it seems unnecessary.
logger.log(
Level.WARNING,
"Error in cache file handling; will re-execute without caching: {}",
e.getMessage());
Item<M> rerequested = itemBuilder.buildItem();
return new CacheResult<>(rerequested.stream(), rerequested.metadata());
}
}
/**
* IOExceptions thrown by the HTTP request are propagated; IOExceptions thrown while storing the
* data in the cache are caught, and the request is re-issued without caching.
*/
private CacheResult<M> makeRequestAndCache(String cacheKey, ItemBuilder<M> itemBuilder)
throws IOException, InterruptedException, ResponseTooLargeException {
throws IOException, InterruptedException, LRUCacheException, ResponseTooLargeException {
assert !cache.containsKey(cacheKey) : "Cache should not contain key " + cacheKey;
Item<M> item = itemBuilder.buildItem();
@ -134,21 +148,29 @@ public class LRUCache<M> {
return getResultForCacheEntry(cacheKey);
} catch (IOException e) {
logger.log(
Level.WARNING,
"Failure storing cache entry; will re-execute without caching: {}",
e.getMessage());
// Re-issue the request since we don't know if we've consumed any of the response.
Item<M> rerequested = itemBuilder.buildItem();
return new CacheResult<>(rerequested.stream(), rerequested.metadata());
// Throw this to re-issue the request since we don't know if we've consumed any of the
// response.
throw new LRUCacheException("Failure storing cache entry", e);
}
}
/** Mark cache entry used and return a stream reading from the cache file. */
private CacheResult<M> getResultForCacheEntry(String cacheKey) throws IOException {
/**
* Mark cache entry used and return a stream reading from the cache file.
*
* <p>If the file has been deleted, an LRUCacheException is thrown, causing .makeRequest to
* re-issue the request without caching.
*/
private CacheResult<M> getResultForCacheEntry(String cacheKey)
throws IOException, LRUCacheException {
var cacheFile = cache.get(cacheKey).responseData;
if (!cacheFile.exists()) {
removeCacheEntry(cacheKey, cache.get(cacheKey));
throw new LRUCacheException("Missing cache file " + cacheFile.getPath());
}
markCacheEntryUsed(cacheKey);
return new CacheResult<>(
new FileInputStream(cache.get(cacheKey).responseData), cache.get(cacheKey).metadata());
return new CacheResult<>(new FileInputStream(cacheFile), cache.get(cacheKey).metadata());
}
/**
@ -217,8 +239,11 @@ public class LRUCache<M> {
/** Remove a cache entry: from `cache`, `lastUsed`, and the filesystem. */
private void removeCacheEntry(Map.Entry<String, CacheEntry<M>> toRemove) {
var key = toRemove.getKey();
var value = toRemove.getValue();
removeCacheEntry(toRemove.getKey(), toRemove.getValue());
}
/** Remove a cache entry: from `cache`, `lastUsed`, and the filesystem. */
private void removeCacheEntry(String key, CacheEntry<M> value) {
cache.remove(key);
lastUsed.remove(key);
removeCacheFile(key, value);
@ -305,6 +330,15 @@ public class LRUCache<M> {
cache.values().stream().map(CacheEntry::size).collect(Collectors.toList()));
}
/** Public for testing. */
public List<String> getFiles() {
return new ArrayList<>(
cache.values().stream()
.map(CacheEntry::responseData)
.map(f -> f.getPath())
.collect(Collectors.toList()));
}
/** Public for testing. */
public LRUCacheSettings getSettings() {
return settings;
@ -344,4 +378,21 @@ public class LRUCache<M> {
private final Comparator<Map.Entry<String, CacheEntry<M>>> cacheEntryLRUComparator =
Comparator.comparing(me -> lastUsed.get(me.getKey()));
/** Represents an internal error in creating or accessing the cache file. */
private static class LRUCacheException extends Exception {
public LRUCacheException(String errorMessage) {
super(errorMessage);
}
public LRUCacheException(String errorMessage, Throwable cause) {
super(errorMessage, cause);
}
public String getMessage() {
var cause = getCause();
var causeMessage = cause == null ? "" : ": " + cause.getMessage();
return super.getMessage() + causeMessage;
}
}
}

View File

@ -176,17 +176,13 @@ public final class EnsoSecretHelper extends SecretValueResolver {
}
}
private static EnsoHTTPResponseCache getOrCreateCache() {
public static EnsoHTTPResponseCache getOrCreateCache() {
if (cache == null) {
cache = new EnsoHTTPResponseCache();
}
return cache;
}
public static EnsoHTTPResponseCache getCache() {
return cache;
}
private static final Comparator<Pair<String, String>> headerNameComparator =
Comparator.comparing((Pair<String, String> pair) -> pair.getLeft())
.thenComparing(Comparator.comparing(pair -> pair.getRight()));

View File

@ -122,7 +122,7 @@ add_specs suite_builder =
action
get_lru_cache =
EnsoSecretHelper.getCache.getLRUCache
EnsoSecretHelper.getOrCreateCache.getLRUCache
get_num_response_cache_entries =
get_lru_cache.getNumEntries
@ -137,15 +137,19 @@ add_specs suite_builder =
counts = with_counts action
counts . should_equal expected_counts frames_to_skip=1
get_cache_files : Vector Text
get_cache_files -> Vector Text =
Vector.from_polyglot_array EnsoSecretHelper.getOrCreateCache.getLRUCache.getFiles . sort Sort_Direction.Ascending
get_cache_file_sizes : Vector Integer
get_cache_file_sizes -> Vector Integer =
Vector.from_polyglot_array EnsoSecretHelper.getCache.getLRUCache.getFileSizes . sort Sort_Direction.Ascending
Vector.from_polyglot_array EnsoSecretHelper.getOrCreateCache.getLRUCache.getFileSizes . sort Sort_Direction.Ascending
# Craetes a new cache each time, then resets it at the end
with_lru_cache lru_cache ~action =
reset = EnsoSecretHelper.getCache.setLRUCache LRUCache.new
reset = EnsoSecretHelper.getOrCreateCache.setLRUCache LRUCache.new
Panic.with_finalizer reset <|
EnsoSecretHelper.getCache.setLRUCache lru_cache
EnsoSecretHelper.getOrCreateCache.setLRUCache lru_cache
action
# Craetes a new cache each time, then resets it at the end
@ -462,44 +466,44 @@ add_specs suite_builder =
Test_Environment.unsafe_with_environment_override "ENSO_LIB_HTTP_CACHE_MAX_FILE_SIZE_MB" "8" <|
Test_Environment.unsafe_with_environment_override "ENSO_LIB_HTTP_CACHE_MAX_TOTAL_CACHE_LIMIT" "30" <|
with_default_cache <|
EnsoSecretHelper.getCache.getLRUCache.getSettings.getMaxFileSize . should_equal (8 * 1024 * 1024)
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal (30 * 1024 * 1024)
EnsoSecretHelper.getOrCreateCache.getLRUCache.getSettings.getMaxFileSize . should_equal (8 * 1024 * 1024)
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal (30 * 1024 * 1024)
group_builder.specify "Should be able to set the max file size and total cache size (as a percentage) via environment variable, and track changes to available disk space" <|
Test_Environment.unsafe_with_environment_override "ENSO_LIB_HTTP_CACHE_MAX_FILE_SIZE_MB" "8" <|
Test_Environment.unsafe_with_environment_override "ENSO_LIB_HTTP_CACHE_MAX_TOTAL_CACHE_LIMIT" "10%" <|
with_mocks _-> disk_space->
EnsoSecretHelper.getCache.getLRUCache.getSettings.getMaxFileSize . should_equal (8 * 1024 * 1024)
EnsoSecretHelper.getOrCreateCache.getLRUCache.getSettings.getMaxFileSize . should_equal (8 * 1024 * 1024)
disk_space.mocked 300
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal 30
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal 30
disk_space.mocked 400
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal 40
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal 40
group_builder.specify "Includes the existing cache files in the total cache size calculation, for a percentage total cache limit" pending=pending_has_url <| Test.with_retries <|
with_config 1000 (TotalCacheLimit.Percentage.new 0.5) _-> disk_space->
disk_space.mocked 100
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal 50
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal 50
fetch_n 30
disk_space.mocked 70
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal 50
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal 50
fetch_n 20
disk_space.mocked 50
get_num_response_cache_entries . should_equal 2
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal 50
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal 50
fetch_n 10
disk_space.mocked 70
get_num_response_cache_entries . should_equal 2
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal 50
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal 50
group_builder.specify "Total cache size, specified in MB, should not go over the percentage hard limit" <|
with_config 1000 (TotalCacheLimit.Bytes.new 200) _-> disk_space->
disk_space.mocked 100
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal 90
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal 90
group_builder.specify "Total cache size, specified as a percentage, should not go over the percentage hard limit" <|
with_config 1000 (TotalCacheLimit.Percentage.new 0.95) _-> disk_space->
disk_space.mocked 100
EnsoSecretHelper.getCache.getLRUCache.getMaxTotalCacheSize . should_equal 90
EnsoSecretHelper.getOrCreateCache.getLRUCache.getMaxTotalCacheSize . should_equal 90
group_builder.specify "Falls back to the default if an environment variable is incorrectly formatted" <|
Test_Environment.unsafe_with_environment_override "ENSO_LIB_HTTP_CACHE_MAX_FILE_SIZE_MB" "abcd" <|
@ -512,3 +516,16 @@ add_specs suite_builder =
LRUCache.new . getSettings . getTotalCacheLimit . should_equal (TotalCacheLimit.Percentage.new 0.2)
Test_Environment.unsafe_with_environment_override "ENSO_LIB_HTTP_CACHE_MAX_TOTAL_CACHE_LIMIT" "101%" <|
LRUCache.new . getSettings . getTotalCacheLimit . should_equal (TotalCacheLimit.Percentage.new 0.2)
group_builder.specify "Reissues the request if the cache file disappears" pending=pending_has_url <| Test.with_retries <|
with_default_cache <|
url = base_url_with_slash+'test_download?max-age=16&length=10'
result0 = HTTP.fetch url . decode_as_text
result0.length . should_equal 10
HTTP.fetch url . decode_as_text . should_equal result0
get_num_response_cache_entries . should_equal 1
get_cache_files.map (f-> File.new f . delete)
result1 = HTTP.fetch url . decode_as_text
result1 . should_not_equal result0
result1.length . should_equal 10