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.

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

ShallowEtagHeaderFilter breaks SockJS and HTTP streaming async responses [SPR-12960] #17552

Closed
spring-issuemaster opened this issue Apr 27, 2015 · 18 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Apr 27, 2015

Alexander Zagumennikov opened SPR-12960 and commented

ShallowEtagHeaderFilter wraps response into ContentCachingResponseWrapper that does not pass body data to wrapped response body.
If body data is written asynchronously after filter completes processing, data never goes to client.

This problem was found when using Spring WebSocket with SockJS in fallback mode. xhr-polling and xhr-streaming fallback options do not work with ShallowEtagHeaderFilter.

ShallowEtagHeaderFilter performs some checks to detect asynchronous request via WebAsyncManager, but WebAsyncManager seems to work only with Spring MVC annotated controllers, because I found only one usage of WebAsyncManager#setAsyncWebRequest - in RequestMappingHandlerAdapter.

Please, let me know if you need more details or examples to reproduce the issue.

Thanks


Affects: 4.1.3

Referenced from: commits 49e9057

0 votes, 6 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 27, 2015

Rossen Stoyanchev commented

In SockJS we are simulating WebSocket interaction, sending many messages over a possibly prolonged period of time. What is the reason why you want to use ShallowEtagHeaderFilter in front of that? The ShallowEtagHeaderFilter does what it's supposed to which is to buffer the response and prevent it from being written until the end when it can compute a hash and decide whether to allow writing or send a 304 instead.

ShallowEtagHeaderFilter does work with async requests where it makes sense such as when a controller returns a DeferredResult or Callable from a controller method. Although the controller does its work asynchronously, eventually it provides a result, so the request is dispatched back into the container, and processing and completes with the ShallowEtagHeaderFilter deciding whether to write to the body or return a 304.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 27, 2015

Alexander Zagumennikov commented

I configured ShallowEtagHeaderFilter to apply to whole application, I was expecting that it won't break anything, and will save bandwidth where possible.
It took quite a lot of time to figure out why SockJS fallback doesn't work in my application, I think that it should be at least documented that ShallowEtagHeaderFilter breaks async responses (excluding request mapping method that returns DeferredResult or Callable).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 29, 2015

Rossen Stoyanchev commented

This is a good point. We should in the very least document it but also see if there is anything more we can do. SockJS as well as the HTTP streaming options coming in 4.2 are basically mutually exclusive with use of the ShallowEtagHeaderFilter. The filter has no way of knowing how an async request will be used but the code that starts async processing knows that ShallowEtagHeaderFilter and any other cases where the response may be buffered should be turned off.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 29, 2015

Alexander Zagumennikov commented

Maybe we can add some listener to detect when response switches to async mode, and then tell ContentCachingResponseWrapper not to cache body, but to pass it to original response.
Or we can wrap request and override startAsync method.
Or we can just check if async operation is started after filterChain.doFilter.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 30, 2015

Rossen Stoyanchev commented

Yes, we need some switch in ContentCachingResponseWrapper to turn off caching. However it's not enough to detect when async handling starts because in some cases (like DeferredResult), the filter should still apply. It's only when using the response for streaming as opposed to producing a result and then completing processing. I think we might be able to do something with checking the java.servlet.DispatcherType in ContentCachingResponseWrapper. I'll experiment with it.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 4, 2015

Rossen Stoyanchev commented

I configured ShallowEtagHeaderFilter to apply to whole application, I was expecting that it won't break anything, and will save bandwidth where possible.

This should work now as expected. Please give it a try.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 5, 2015

Alexander Zagumennikov commented

Unfortunately, I can't get sockjs connection working via xhr-polling or xhr-streaming even without ShallowEtagHeaderFilter with spring 4.2.0.BUILD-SNAPSHOT
I'm getting error in js: 'Uncaught TypeError: Cannot read property 'split' of undefined' in sockjs 'Client.prototype._setupHeartbeat' function on line 'ref1 = headers["heart-beat"].split(",");'
Seems that server doesn't send 'heart-beat' header

I use sockjs client 0.3.4
Spring 4.1.3 works well without ShallowEtagHeaderFilter

JS log:

Opening Web Socket...
Web Socket Opened...
>>> CONNECT
accept-version:1.1,1.0
heart-beat:10000,10000


<<< CONNECTED
version:1.1
user-name:zagumennikov@haulmont.com


connected to server undefined
Uncaught TypeError: Cannot read property 'split' of undefined
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 5, 2015

Brian Clozel commented

Hey Alexander Zagumennikov,

This issue is related to #15582 - I ran into the same problem.
I've got a fix for this - let me get back to you as soon as it's in the latest SNAPSHOT version so you can resume your tests.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 5, 2015

Brian Clozel commented

Alexander Zagumennikov - it's now deployed as the latest SNAPSHOT!
Could you try again please?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2015

Alexander Zagumennikov commented

I can confirm that it works with latest 4.2.0 snapshot. Thanks for prompt response!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 17, 2016

Christopher Savory commented

Brian Clozel and Rossen Stoyanchev we had this same issue last year and upgrading to Spring Boot 1.3 (SF 4.2.3.RELEASE) fixed the issue. Now it appears to be back after upgrading to Boot 1.3.2 (SF 4.2.4.RELEASE).

The way the problem manifests in our game that uses XHR on AWS is that if two people are playing the game at the same time, one HTTP Session will overwrite the other and now the user is logged in as the other person.

I noticed that Juergen Hoeller made some changes to ShallowEtagHeaderFilter last Sept and Dec, but it's very difficult to tell which release they were included in.

Any ideas on what might be causing this issue to come back?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 17, 2016

Juergen Hoeller commented

The only change made between 4.2.3 and 4.2.4 was the use of HttpMethod.matches over a String comparison. Not sure whether this has anything to do with the regression you're seeing...

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 17, 2016

Christopher Savory commented

Thanks Juergen Hoeller for the info. I would like to create a small project to recreate this bug locally, but it only happens when deployed to the AWS environment. On AWS the SockJS client first tries a websocket and fails. Then switches over to XHR.

To try to reproduce this locally, I added this to my SockJS config:

protocols_whitelist : ['xdr-streaming', 'xhr-streaming', 'iframe-eventsource', 'xdr-polling', 'xhr-polling']

But that still doesn't reproduce the issue locally. The session overwrite issue only goes away if I disable the ShallowEtagHeaderFilter.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 17, 2016

Brian Clozel commented

Is there a CDN / caching proxy involved in your AWS setup?
Maybe it's caching a response it shouldn't, serving the same response to a different user?

In fact, why is this filter configured on those parts of the application - what do you intend to cache in that case?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 17, 2016

Christopher Savory commented

Brian Clozel we are using a CDN, but the websocket (XHR) requests go straight from the browser through the ELB to the embedded Tomcat.

In fact, why is this filter configured on those parts of the application - what do you intend to cache in that case?

This is how we are configuring the Filter (globally):

@Bean
public ShallowEtagHeaderFilter shallowEtagHeaderFilter() {
     return new ShallowEtagHeaderFilter();
}

It never occured to me that I could just have this Filter just skip over our WebSocket url pattern as we don't need eTag support at all for those. Now I have it configured like this and everything seems to be fine.

	@Bean
	public ShallowEtagHeaderFilter shallowEtagHeaderFilter() {
		return new ShallowEtagHeaderFilter() {

			private final String[] PATTERNS_TO_SKIP = { WebSocketConfig.WEB_SOCKET_ENDPOINT };

			@Override
			protected boolean shouldNotFilter( HttpServletRequest request ) throws ServletException {
				return StringUtils.startsWithAny( request.getRequestURI(), PATTERNS_TO_SKIP );
			}
		};
	}

I still think there is a bug here somewhere in the Spring Messaging WebSocket / ETagFilter code. And it's a pretty serious one since http sessions are being overwritten. This workaround will work for us in the meantime.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 19, 2016

Rossen Stoyanchev commented

In the original fix AbstractHttpSession (and a couple of others) explicitly disable the shallow eTag response handling. I can't see why it would be affect by the change from 4.2.3 to 4.2.4. You could vary just the Spring Framework version to see if that makes a difference. How easy is it for you to reproduce the issue in AWS?

We could try to move the call to disable shallow eTag that's in AbstractHttpSession to happen a little earlier, e.g. in SockJsHttpRequestHandler. Perhaps there are some error conditions that currently prevents it from getting executed.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 2, 2016

Christopher Savory commented

Rossen Stoyanchev it's very difficult to repro on AWS. I can only do it about 1 out of 20 times and I have to play the same game with two different users at the same time using two browsers. I haven't be able to debug since I can't do it locally.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 2, 2016

Christopher Savory commented

I actually went ahead and removed ShallowEtagHeaderFilter from our app altogether since ResourceHttpRequestHandler is adding ETags for versioned resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.