Add the ability to limit ContentCachingResponseWrapper cache size

[vpavic]

It would be nice if ContentCachingResponseWrapper could support setting a limit to its cache so that response body is cached only up to this limit. This could be beneficial for the existing ShallowEtagHeaderFilter and other use case that leverage ContentCachingResponseWrapper.

See spring-projects/spring-boot#43697 (comment) and the discussion around it for background of this enhancement request.

[bclozel]

If I understand correctly, this new feature means that developers would configure a threshold property (in bytes) on the filter instance and this would change its behavior at runtime. If the response body is larger than the threshold, the wrapper writes all cached content to the actual output with copyBodyToResponse and then resumes caching content.

From there, developers can only assume that the cached content is a piece of the actual response body and they have no clue about the total size of the response body. Right now, if the cached content exceeds 2Gb an exception is thrown. This also means that in this case the "Content-Length" response headers cannot be written to the response as the response would be committed already.

This would developers in a position where ContentCachingResponseWrapper has two very different behavior modes:

1. caching the entire content and writing content-length headers, failing otherwise
2. caching some part of the end of the content (this would not be a sliding window - if the response size is MAX + 1, then the cached content would be of size "1") and not setting the Content-Length header anymore.

I don't really like this proposal in this current state and I'm not sure we want to push ContentCachingResponseWrapper usage because of its existing limitations.

[vpavic]

If the response body is larger than the threshold, the wrapper writes all cached content to the actual output with copyBodyToResponse and then resumes caching content.

I didn't yet take a deep look into this, but I was thinking something along these lines:

• code that writes cache (presumably custom ServletOutputStream) is aware of the threshold and tracks whether it is reached
• when it's reached, it writes everything cached so far to the underlying ServletOutputStream and stops caching (this could be logged as warning)
• further writes to custom ServletOutputStream skip cache and delegate to the underlying ServletOutputStream

In this case, when copyBodyToResponse is called at the end it will be a no-op (since cache will be empty). I think this approach could remove the need for that exception when content length exceeds the current limit. Default cache limit could then be much smaller than the current hard limit as it wouldn't break request processing, but the feature that depends on the cache (for example, ETag filter) wouldn't work but logged warning from the second point above would ensure developers are aware when that happens.

I acknowledge that I might be missing something, because as mentioned, my analysis of this so far wasn't too deep.

I'm not sure we want to push ContentCachingResponseWrapper usage because of its existing limitations.

Can you clarify this part a bit? Does it mean that usage of ContentCachingResponseWrapper will be discouraged?

[bclozel]

In this case, when copyBodyToResponse is called at the end it will be a no-op (since cache will be empty). I think this approach could remove the need for that exception when content length exceeds the current limit. Default cache limit could then be much smaller than the current hard limit as it wouldn't break request processing, but the feature that depends on the cache (for example, ETag filter) wouldn't work but logged warning from the second point above would ensure developers are aware when that happens.

In this case, from a developer perspective, calling getContentAsByteArray would yield an empty byte array and you wouldn't know if nothing was written or if we went over the threshold.

Can you clarify this part a bit? Does it mean that usage of ContentCachingResponseWrapper will be discouraged?

No need to over-interpret my comment: there are well-known limitations to caching the response content. We're not about to deprecate it, but I think it's sane to question further extensions there here for a very specific use case in Spring Boot. For the ETag use case, I think that HTTP trailer fields might be a better alternative in the future because this approach could get rid of the caching limitations.

[vpavic]

In this case, from a developer perspective, calling getContentAsByteArray would yield an empty byte array and you wouldn't know if nothing was written or if we went over the threshold.

The flag that tracks whether threshold was reached could easily be exposed, so code that uses the wrapper could check that before attempting to read the cached content.

All that aside, since you are aware of the discussion in spring-projects/spring-boot#43697, do you maybe have an alternative suggestion or feedback on the latest state over there? Since LiveReload support has recently been made an opt-in feature (spring-projects/spring-boot#47387) perhaps this isn't needed, especially if also from Framework's perspective it isn't considered an improvement. I only raised this issue because such idea was thrown into the mix over there.