Skip to content

Commit

Permalink
Fix HTTP cache assertion failure for small disks (#11738)
Browse files Browse the repository at this point in the history
(cherry picked from commit d79b421)
  • Loading branch information
GregoryTravis authored and jdunkerley committed Dec 10, 2024
1 parent d774465 commit 6d24ed0
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -560,24 +560,25 @@ type Out_Of_Range
## Indiciates that the response from a remote endpoint is over the size limit.
type Response_Too_Large
## PRIVATE
Error limit:Integer
Error actual_size:Integer|Nothing limit:Integer

## PRIVATE
Create a human-readable version of the error.
to_display_text : Text
to_display_text self =
suggestion = " Use `Data.fetch` with `cache_policy=No_Cache`, or use `Data.download` to fetch the data to a local file, and `Data.read` to read the file."
"Response too large: repsonse size is over the limit ("+self.limit.to_text+")" + suggestion
actual_size_message = if self.actual_size == Nothing then "" else " "+self.actual_size.to_text
"Response too large: response size"+actual_size_message+" is over the limit "+self.limit.to_text+"."+suggestion

## PRIVATE
to_text : Text
to_text self =
"(Response_Too_Large (limit = "+self.limit.to_text+")" + ")"
"(Response_Too_Large (actual_size = "+self.actual_size.to_text+") (limit = "+self.limit.to_text+"))"

## PRIVATE
Convert the Java exception to an Enso dataflow error.
handle_java_exception ~action =
Panic.catch ResponseTooLargeException action (cause-> Error.throw (Response_Too_Large.Error cause.payload.getLimit))
Panic.catch ResponseTooLargeException action (cause-> Error.throw (Response_Too_Large.Error cause.payload.getActualSize cause.payload.getLimit))

## Indicates that some operation relies on equality on floating-point values,
which is not recommended.
Expand Down
26 changes: 17 additions & 9 deletions std-bits/base/src/main/java/org/enso/base/cache/LRUCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,18 @@ private CacheResult<M> makeRequestAndCache(String cacheKey, ItemBuilder<M> itemB
return new CacheResult<>(item.stream(), item.metadata());
}

long maxAllowedDownloadSize = getMaxAllowedDownloadSize();

// If we have a content-length, clear up enough space for that. If not,
// then clear up enough space for the largest allowed file size.
long maxFileSize = settings.getMaxFileSize();
// then clear up enough space for the largest allowed size.
if (item.sizeMaybe.isPresent()) {
long size = item.sizeMaybe().get();
if (size > maxFileSize) {
throw new ResponseTooLargeException(maxFileSize);
if (size > maxAllowedDownloadSize) {
throw new ResponseTooLargeException(size, maxAllowedDownloadSize);
}
makeRoomFor(size);
} else {
makeRoomFor(maxFileSize);
makeRoomFor(maxAllowedDownloadSize);
}

try {
Expand Down Expand Up @@ -185,15 +186,14 @@ private File downloadResponseData(String cacheKey, Item item)
var outputStream = new FileOutputStream(temp);
boolean successful = false;
try {
// Limit the download to getMaxFileSize().
long maxFileSize = settings.getMaxFileSize();
boolean sizeOK = Stream_Utils.limitedCopy(inputStream, outputStream, maxFileSize);
long maxAllowedDownloadSize = getMaxAllowedDownloadSize();
boolean sizeOK = Stream_Utils.limitedCopy(inputStream, outputStream, maxAllowedDownloadSize);

if (sizeOK) {
successful = true;
return temp;
} else {
throw new ResponseTooLargeException(maxFileSize);
throw new ResponseTooLargeException(null, maxAllowedDownloadSize);
}
} finally {
outputStream.close();
Expand Down Expand Up @@ -315,6 +315,14 @@ public long getMaxTotalCacheSize(long currentlyUsed) {
return Long.min(upperBound, totalCacheSize);
}

/**
* Calculate the largest size we can allow, which is the minimum of the max file size and the max
* total cache size.
*/
private long getMaxAllowedDownloadSize() {
return Long.min(settings.getMaxFileSize(), getMaxTotalCacheSize());
}

/** For testing. */
public long getMaxTotalCacheSize() {
return getMaxTotalCacheSize(getTotalCacheSize());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ public LRUCacheSettings(long maxFileSize, TotalCacheLimit.Limit totalCacheLimit)
this.totalCacheLimit = totalCacheLimit;
}

public String toString() {
return "LRUCacheSettings(" + maxFileSize + ", " + totalCacheLimit + ")";
}

/** Uses defaults if the vars are not set. */
public static LRUCacheSettings getDefault() {
return new LRUCacheSettings(parseMaxFileSizeEnvVar(), parseTotalCacheLimitEnvVar());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
package org.enso.base.cache;

public class ResponseTooLargeException extends Exception {
private final Long actualSize;
private final long limit;

public ResponseTooLargeException(long limit) {
super("Response too large: repsonse size is over the limit (" + limit + ")");
public ResponseTooLargeException(Long actualSize, long limit) {
super(
"Response too large: response size"
+ (actualSize == null ? "" : " " + actualSize)
+ " is over the limit "
+ limit);

this.actualSize = actualSize;
this.limit = limit;
}

public long getLimit() {
return limit;
}

public Long getActualSize() {
return actualSize;
}
}
30 changes: 27 additions & 3 deletions test/Table_Tests/src/IO/Fetch_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ add_specs suite_builder =
url = base_url_with_slash+"test_download?length=200"
Data.download url target_file
target_file.read.length . should_equal 200
Data.fetch url . should_fail_with (Response_Too_Large.Error 100)
Data.fetch url . should_fail_with (Response_Too_Large.Error 200 100)

group_builder.specify "Should not cache for methods other than GET" pending=pending_has_url <| Test.with_retries <|
with_default_cache <|
Expand Down Expand Up @@ -390,12 +390,12 @@ add_specs suite_builder =

group_builder.specify "Will not cache a file with a content length greater than the single file limit" pending=pending_has_url <| Test.with_retries <|
with_config 100 (TotalCacheLimit.Bytes.new 1000) _-> _->
fetch_n 110 . should_fail_with (Response_Too_Large.Error 100)
fetch_n 110 . should_fail_with (Response_Too_Large.Error 110 100)

group_builder.specify "Will not cache a file without a content length, but which is greater than the single file limit" pending=pending_has_url <| Test.with_retries <|
with_config 100 (TotalCacheLimit.Bytes.new 1000) _-> _->
url = base_url_with_slash+'test_download?omit-content-length=1&length=110'
Data.fetch url . should_fail_with (Response_Too_Large.Error 100)
Data.fetch url . should_fail_with (Response_Too_Large.Error Nothing 100)

group_builder.specify "Will make room for the largest possible file, if the server does not provide a content-length" pending=pending_has_url <| Test.with_retries <|
with_config 50 (TotalCacheLimit.Bytes.new 100) _-> _->
Expand Down Expand Up @@ -457,6 +457,30 @@ add_specs suite_builder =
HTTP.fetch headers=headers2 uri
get_num_response_cache_entries . should_equal 2

group_builder.specify "Does not attempt to make room for the maximum file size when that is larger than the total cache size" pending=pending_has_url <| Test.with_retries <|
with_config 1000 (TotalCacheLimit.Bytes.new 100) _-> _->
url = base_url_with_slash+'test_download?omit-content-length=1&length=10'
Data.fetch url . decode_as_text . should_succeed

group_builder.specify "Limits a single download to the max total cache size when that is smaller than the max allowed file size" pending=pending_has_url <| Test.with_retries <|
with_config 1000 (TotalCacheLimit.Bytes.new 100) _-> _->
url = base_url_with_slash+'test_download?omit-content-length=1&length=200'
Data.fetch url . should_fail_with (Response_Too_Large.Error Nothing 100)

group_builder.specify "Throws Response_Too_Large with a content-length header" pending=pending_has_url <| Test.with_retries <|
with_config 5 (TotalCacheLimit.Bytes.new 1000000) _-> _->
url = base_url_with_slash+'test_download?length=10'
err = Data.fetch url . catch
err . should_equal (Response_Too_Large.Error 10 5)
err.to_display_text . starts_with "Response too large: response size 10 is over the limit 5." . should_be_true

group_builder.specify "Throws Response_Too_Large without a content-length header" pending=pending_has_url <| Test.with_retries <|
with_config 5 (TotalCacheLimit.Bytes.new 1000000) _-> _->
url = base_url_with_slash+'test_download?omit-content-length=1&length=10'
err = Data.fetch url . catch
err . should_equal (Response_Too_Large.Error Nothing 5)
err.to_display_text . starts_with "Response too large: response size is over the limit 5." . should_be_true

group_builder.specify "Cache limits should have defaults" <|
lru_cache = LRUCache.new
lru_cache.getSettings.getMaxFileSize . should_equal (2 * 1024 * 1024 * 1024)
Expand Down

0 comments on commit 6d24ed0

Please sign in to comment.