Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Static resources caching issues with ShallowEtagHeaderFilter and Jetty caching directives #32039

Closed
sebastian-steiner opened this issue Jan 16, 2024 · 3 comments
Closed

Static resources caching issues with ShallowEtagHeaderFilter and Jetty caching directives #32039

sebastian-steiner opened this issue Jan 16, 2024 · 3 comments
Assignees
@poutsma
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone
6.1.4

Comments

@sebastian-steiner
Copy link

Affects: 6.1.2 on Spring Boot 3.2.0

When the ShallowEtagHeaderFilter is enabled along with the jetty web server, static resources fail to load every second time.

If you fetch a static resource under this configuration, it will work fine for the first call. On the second request, with an ETag attached, the server responds with the following error:

<h2>HTTP ERROR 500 java.io.IOException: written 0 &lt; 4 content-length</h2>

URI: | http://localhost:8080/
-- | --
500
java.io.IOException: written 0 < 4 content-length
java.io.IOException: written 0 < 4 content-length


<h3>Caused by:</h3><pre>java.io.IOException: written 0 &lt; 4 content-length
	at org.eclipse.jetty.server.internal.HttpChannelState$ChannelResponse.write(HttpChannelState.java:1182)
	at org.eclipse.jetty.server.Response$Wrapper.write(Response.java:735)
	at org.eclipse.jetty.server.handler.ContextResponse.write(ContextResponse.java:56)
	at org.eclipse.jetty.ee10.servlet.HttpOutput.channelWrite(HttpOutput.java:204)
	at org.eclipse.jetty.ee10.servlet.HttpOutput.complete(HttpOutput.java:435)
	at org.eclipse.jetty.ee10.servlet.ServletContextResponse.completeOutput(ServletContextResponse.java:209)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.handle(ServletChannel.java:553)
	at org.eclipse.jetty.ee10.servlet.ServletHandler.handle(ServletHandler.java:464)
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:571)
	at org.eclipse.jetty.ee10.servlet.SessionHandler.handle(SessionHandler.java:703)
	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:761)
	at org.eclipse.jetty.server.Server.handle(Server.java:179)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:594)
	at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:424)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:322)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:478)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:441)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:293)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:195)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:971)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1201)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1156)
	at java.base/java.lang.Thread.run(Thread.java:1583)
</pre>

It seems that the response is already marked as "Not Modified" and the body cleared by one caching component, while the other still expects it to be unchanged.

Repository with minimal reproducer: https://github.com/sebastian-steiner/spring-cacheissue
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 16, 2024
@jhoeller jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 17, 2024
@poutsma
Copy link
Contributor

poutsma commented Jan 17, 2024

This might be related to jetty/jetty.project#9953.

@poutsma poutsma self-assigned this Jan 17, 2024
@poutsma poutsma added type: bug A general bug for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 18, 2024
@poutsma poutsma modified the milestones: 6.0.17, 6.1.4 Jan 18, 2024
@github-actions github-actions bot added for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x labels Jan 18, 2024
This was referenced Jan 18, 2024
Closed
Closed
@poutsma
Copy link
Contributor

poutsma commented Jan 18, 2024

The reason for this issue is that the response has a Content-Length header that does not correspond to the amount of bytes written, an error that Jetty 12 tracks more strictly. While the ShallowEtagHeaderFilter does filter out the content-length if set through setContentLength, it does not do so when setHeader("Content-Length", x) is used. I will prepare a fix.

@sebastian-steiner
Copy link
Author

Thanks for the explanation and the quick triage!

poutsma added a commit that referenced this issue Jan 18, 2024
@poutsma
Handle Content-Length in ShallowEtagHeaderFilter more robustly
a0f24cf 
This commit ensures that setting the Content-Length through
setHeader("Content-Length", x") has the same effect as calling
setContentLength in the ShallowEtagHeaderFilter. It also filters out
Content-Type headers similarly to Content-Length.

See gh-32039
Closes gh-32051
poutsma added a commit that referenced this issue Jan 18, 2024
@poutsma
Handle Content-Length in ShallowEtagHeaderFilter more robustly
38595c6 
This commit ensures that setting the Content-Length through
setHeader("Content-Length", x") has the same effect as calling
setContentLength in the ShallowEtagHeaderFilter. It also filters out
Content-Type headers similarly to Content-Length.

See gh-32039
Closes gh-32050
@jhoeller jhoeller removed the for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x label Feb 23, 2024
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Feb 24, 2024
@sbrannen
Honor Content-[Type|Length] headers from wrapped response again
c26ef78 
Commit 375e0e6 introduced a regression in
ContentCachingResponseWrapper (CCRW). Specifically, CCRW no longer
honors Content-Type and Content-Length headers that have been set on
the wrapped response and returns null for those header values if they
have not been set directly on the CCRW.

This commit fixes this regression as follows.

- The Content-Type and Content-Length headers set in the wrapped
  response are honored in getContentType(), containsHeader(),
  getHeader(), and getHeaders() unless those headers have been set
  directly on the CCRW.

- In copyBodyToResponse(), the Content-Type in the wrapped response is
  only overridden if the the Content-Type has been set directly on the
  CCRW.

See spring-projectsgh-32039
Closes spring-projectsgh-32317
sbrannen added a commit that referenced this issue Feb 24, 2024
@sbrannen
Honor Content-[Type|Length] headers from wrapped response again
5680d20 
Commit 375e0e6 introduced a regression in
ContentCachingResponseWrapper (CCRW). Specifically, CCRW no longer
honors Content-Type and Content-Length headers that have been set on
the wrapped response and returns null for those header values if they
have not been set directly on the CCRW.

This commit fixes this regression as follows.

- The Content-Type and Content-Length headers set in the wrapped
  response are honored in getContentType(), containsHeader(),
  getHeader(), and getHeaders() unless those headers have been set
  directly on the CCRW.

- In copyBodyToResponse(), the Content-Type in the wrapped response is
  only overridden if the Content-Type has been set directly on the CCRW.

See gh-32039
Closes gh-32317
sbrannen added a commit that referenced this issue Feb 25, 2024
@sbrannen
Return unique set from ContentCachingResponseWrapper.getHeaderNames()
be45481 
Prior to this commit, getHeaderNames() returned duplicates for the
Content-Type and Content-Length headers if they were set in the wrapped
response as well as in ContentCachingResponseWrapper.

This commit fixes that by returning a unique set from getHeaderNames().

In addition, this commit introduces a new test in
ContentCachingResponseWrapperTests to verify the expected behavior for
Content-Type and Content-Length headers that are set in the wrapped
response as well as in ContentCachingResponseWrapper.

See gh-32039
See gh-32317
sbrannen added a commit that referenced this issue Feb 25, 2024
@sbrannen
Honor Content-[Type|Length] headers from wrapped response again
ca602ef 
Commit 375e0e6 introduced a regression in
ContentCachingResponseWrapper (CCRW). Specifically, CCRW no longer
honors Content-Type and Content-Length headers that have been set in
the wrapped response and now incorrectly returns null for those header
values if they have not been set directly in the CCRW.

This commit fixes this regression as follows.

- The Content-Type and Content-Length headers set in the wrapped
  response are honored in getContentType(), containsHeader(),
  getHeader(), and getHeaders() unless those headers have been set
  directly in the CCRW.

- In copyBodyToResponse(), the Content-Type in the wrapped response is
  only overridden if the Content-Type has been set directly in the CCRW.

Furthermore, prior to this commit, getHeaderNames() returned duplicates
for the Content-Type and Content-Length headers if they were set in the
wrapped response as well as in CCRW.

This commit fixes that by returning a unique set from getHeaderNames().

This commit also updates ContentCachingResponseWrapperTests to verify
the expected behavior for Content-Type and Content-Length headers that
are set in the wrapped response as well as in CCRW.

See gh-32039
See gh-32317
Closes gh-32321
sbrannen added a commit that referenced this issue Feb 25, 2024
@sbrannen
Honor Content-[Type|Length] headers from wrapped response again
380f5d5 
Commit 375e0e6 introduced a regression in
ContentCachingResponseWrapper (CCRW). Specifically, CCRW no longer
honors Content-Type and Content-Length headers that have been set in
the wrapped response and now incorrectly returns null for those header
values if they have not been set directly in the CCRW.

This commit fixes this regression as follows.

- The Content-Type and Content-Length headers set in the wrapped
  response are honored in getContentType(), containsHeader(),
  getHeader(), and getHeaders() unless those headers have been set
  directly in the CCRW.

- In copyBodyToResponse(), the Content-Type in the wrapped response is
  only overridden if the Content-Type has been set directly in the CCRW.

Furthermore, prior to this commit, getHeaderNames() returned duplicates
for the Content-Type and Content-Length headers if they were set in the
wrapped response as well as in CCRW.

This commit fixes that by returning a unique set from getHeaderNames().

This commit also updates ContentCachingResponseWrapperTests to verify
the expected behavior for Content-Type and Content-Length headers that
are set in the wrapped response as well as in CCRW.

See gh-32039
See gh-32317
Closes gh-32322
sbrannen added a commit that referenced this issue Feb 26, 2024
@sbrannen
Do not cache Content-Type in ContentCachingResponseWrapper
cca440e 
Based on feedback from several members of the community, we have
decided to revert the caching of the Content-Type header that was
introduced in ContentCachingResponseWrapper in 375e0e6.

This commit therefore completely removes Content-Type caching in
ContentCachingResponseWrapper and updates the existing tests
accordingly.

To provide guards against future regressions in this area, this commit
also introduces explicit tests for the 6 ways to set the content length
in ContentCachingResponseWrapper and modifies a test in
ShallowEtagHeaderFilterTests to ensure that a Content-Type header set
directly on ContentCachingResponseWrapper is propagated to the
underlying response even if content caching is disabled for the
ShallowEtagHeaderFilter.

See gh-32039
Closes gh-32317
sbrannen added a commit that referenced this issue Feb 28, 2024
@sbrannen
Do not cache Content-Type in ContentCachingResponseWrapper
d1b3107 
Based on feedback from several members of the community, we have
decided to revert the caching of the Content-Type header that was
introduced in ContentCachingResponseWrapper in 375e0e6.

This commit therefore completely removes Content-Type caching in
ContentCachingResponseWrapper and updates the existing tests
accordingly.

To provide guards against future regressions in this area, this commit
also introduces explicit tests for the 6 ways to set the content length
in ContentCachingResponseWrapper and modifies a test in
ShallowEtagHeaderFilterTests to ensure that a Content-Type header set
directly on ContentCachingResponseWrapper is propagated to the
underlying response even if content caching is disabled for the
ShallowEtagHeaderFilter.

See gh-32039
See gh-32317
Closes gh-32321
sbrannen added a commit that referenced this issue Feb 28, 2024
@sbrannen
Do not cache Content-Type in ContentCachingResponseWrapper
464fa7e 
Based on feedback from several members of the community, we have
decided to revert the caching of the Content-Type header that was
introduced in ContentCachingResponseWrapper in 375e0e6.

This commit therefore completely removes Content-Type caching in
ContentCachingResponseWrapper and updates the existing tests
accordingly.

To provide guards against future regressions in this area, this commit
also introduces explicit tests for the 6 ways to set the content length
in ContentCachingResponseWrapper and modifies a test in
ShallowEtagHeaderFilterTests to ensure that a Content-Type header set
directly on ContentCachingResponseWrapper is propagated to the
underlying response even if content caching is disabled for the
ShallowEtagHeaderFilter.

See gh-32039
See gh-32317
Closes gh-32322
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@poutsma poutsma

Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Milestone
6.1.4
Development

No branches or pull requests
4 participants
@poutsma @jhoeller @sebastian-steiner @spring-projects-issues