Response is committed before Interceptor postHandle invoked [SPR-9226]

[spring-projects-issues]

Stuart Gunter opened SPR-9226 and commented

In certain circumstances, the response is committed before the Interceptor postHandle method is invoked. This appears to be caused when the HandlerMethod is annotated with @ResponseBody.

Steps to reproduce:

1. Create a Controller method annotated with @ResponseBody.
2. Configure an Interceptor for this path (or all path mappings)
3. Submit a request to the path above, with a breakpoint set in the Interceptor's postHandle method.
4. Check the value of response.isCommitted()

Expected:
Response should not be committed

Actual:
Response is committed.

Implications:
Interceptors are unable to modify the response for @ResponseBody HandlerMethods. This prevents Interceptors from being able to add headers to the response.

---

Affects: 3.1.1

Issue Links:

• StringHttpMessageConverter flush out response immediately before execute posthandler [SPR-16046] #20595 StringHttpMessageConverter flush out response immediately before execute posthandler ("is duplicated by")
• Make @RequestMapping inject the negotiated MediaType [SPR-9980] #14614 Make @RequestMapping inject the negotiated MediaType
• Provide better intercepting model for controllers returning @ResponseBody [SPR-10859] #15486 Provide better intercepting model for controllers returning @ResponseBody ("is superseded by")

2 votes, 7 watchers

[spring-projects-issues]

Stuart Gunter commented

I forgot to explain my reason for marking this as Major priority.

From what I can tell, there is no easy workaround for this problem. All other methods that could be decorated with AOP are either final or not accessible. This leaves the possibility of using AspectJ load-time weaving, but I don't think this should be necessary when the framework provides interceptors for this purpose.

If an alternative is available, I'd be happy to give it a try.

[spring-projects-issues]

Rossen Stoyanchev commented

Have you considered using view resolution with the ContentNegotiatingViewResolver?

[spring-projects-issues]

Stuart Gunter commented

Thanks Rossen. I hadn't checked that, but will take a look.

The problem though is that I'm writing a library that will be used by other applications, so I have no control over how they handle their view rendering. If I impose use of the ContentNegotiatingViewResolver, it might limit the scenarios where my library can be used.

Is there a way to provide functionality described above without knowing / caring how the view is rendered? Interceptors are well understood by Spring users and have traditionally been the way to achieve this, so I'm hoping that will still work.

Will definitely try out the CNVR though, just as a fallback position if nothing else works. Not sure it cleanly maps to my use case, but it's definitely worth a shot.

[spring-projects-issues]

Rossen Stoyanchev commented

I see. I wonder if it varies by content type, i.e. depending on which HttpMessageConverter is used? Looking at AbstractHttpMessageConverter.write(..), it flushes the body but that shouldn't commit the response. So it depends on the implementation details of writeInternal(..).

How do you determine what headers to write? I'm just wondering if there isn't an option to set them in preHandle(). For what it's worth in Spring 3.1 with the new HandlerMapping and HandlerAdapter for annotated controllers you can cast the handler to a HandlerMethod, so you can see exactly which method will be invoked, how it's annotated, etc.

[spring-projects-issues]

Stuart Gunter commented

Yes, it probably does make sense to use preHandle if possible. I was hoping to put all the functionality in postHandle, but I can probably split it between pre/post. It'll need a bit of refactoring, but it should be able to work as you say.

Here's the project where I came across the issue: https://github.com/stuartgunter/rep-tools/blob/master/spring-rep/src/main/java/org/stuartgunter/rep/interceptor/RepInterceptor.java

I'm already using Spring 3.1 with HandlerMethod. Excellent stuff!!!

[spring-projects-issues]

Rossen Stoyanchev commented

Thanks for the link. Let us know how it goes.

I would still like to ask which content type(s) you tested with. I expect the behavior varies by HttpMessageConverter implementation. Specifically I think some HttpMessageConverter implementations won't cause the response to be committed early.

Something else I noticed is the ShallowEtagHeaderFilter. It manipulates the response even later than HandlerInterceptors and it works becase it wraps the response to delay the writing of response headers and the response body. However, it does that for reasons independent of how HttpMessageConverter implementations work. Hence not really applicable to you, just background information.

[spring-projects-issues]

Stuart Gunter commented

I've only tested with "application/json", which in my example uses the StringHttpMessageConverter. I also tried hooking up the MappingJacksonHttpMessageConverter, but this has the same problem of committing the response before Interceptor.postHandle() fires.

I did consider using a filter, but wasn't too keen on manipulating the raw response for the meta tags - adding data to the model seemed to be less risky.

[spring-projects-issues]

Rossen Stoyanchev commented

I'm resolving this for now since the use of message converters implies the request gets handled right after the controller method returns and before postHandler and afterCompletion.

I see that in your latest version you're writing the headers in preHandle and using a request attribute to pass on List<RobotInfo> to postHandle to also update the model. Consider caching the List<RobotInfo> by Method similar to the way we cache the knowledge of @InitBinder and @ModelAttribute methods in RequestMappingHandlerAdapter. Then you can perform a lookup in preHandle and also in postHandle without any performance penalty.

[spring-projects-issues]

whardwick commented

Why was this issue closed? I'm having the same problem, trying to change the content type of the response in the postHandle() method, but the response is already committed, because I'm using @ResponseBody.

The issue I'm trying to address by using the interceptor is that IE doesn't understand content type "application/json" if it wasn't also in the accept header. So, in postHandle() I determine if "application/json" was in the accept header, and if it was not, but the response type is "application/json", I change it to "text/html".

The reason this doesn't work in the preHandle() method is that the content type is not set yet, so I can't determine if it should be changed or not.

[spring-projects-issues]

Rossen Stoyanchev commented

The issue was 'resolved' as opposed to 'closed' which means it can be re-opened. The reason for resolving it is because postHandle is "called after HandlerAdapter actually invoked the handler but before the DispatcherServlet renders the view". In the case of @ResponseBody the request is fully handled by the time postHandle is invoked so effectively it works as designed.

The only way I can see of making sure the response remains not committed is to buffer the response for example through a filter. Very similar to what the ShallowEtagHeaderFilter does.

For your specific case you could wrap the response from a filter to intercept writing the Accept header and modifying it in the process.

[spring-projects-issues]

whardwick commented

My suggestion is that a HandlerInterceptor should actually act as an "interceptor", and postHandle() should occur before the response is committed. Then, afterCompletion() would be the way to deal with the committed response.

[spring-projects-issues]

Rossen Stoyanchev commented

HandlerInterceptor is an "interceptor" and its points of interception are defined very precisely in the Javadoc. With @ResponseBody methods, the body is written during the invocation of the handler and by the time postHandle is called, the response is already committed.

Suppose we were to re-design the HandlerInterceptor contract, an interception point like the one you need would have to be after the headers are set but before the body is written. To understand this point, consider when the response actually gets committed?

It happens when the response is flushed or when the response buffer fills up. It's true the AbstractHttpMessageConverter flushes the response. However, the Jackson JSON library also flushes the response internally by default -- a behavior that can be turned off but the response may still be flushed when the response buffer fills up. You can then try to play with setting the response buffer size but I'm not sure that's the way to go.

My recommendation is to wrap the response from a filter and modify the 'Content-Type' header as soon as it is set.

[spring-projects-issues]

whardwick commented

The reason I had this issue is that when doing an AJAX file upload in IE7/8/9, responding with JSON from the controller method results in a download dialog being presented in the browser (to download the JSON response as a file), which is obviously not the desired result since I want my Javascript to use the response.

The reason this happens is because IE does not pass "application/json" in the Accept header, and so even if you respond with "application/json" in the response, IE will ignore it and present the download dialog.

The solution was to not use @ResponseBody (because of the issues explained above), and to manually set the accept header to be "text/plain" if the request didn't contain "application/json" in the header.

Example:

String accept = request.getHeader("Accept");
if (accept != null && !accept.contains("application/json")) {
    response.setContentType("text/plain");
} else {
    response.setContentType("application/json");
}

[spring-projects-issues]

John Phelan commented

After the controller method but before jackson mapping is a very useful time to intercept handling.
I can't see how a HttpMessageConverter could get access to the handlermethod, and an interceptor will get json(that's probably already flushed?), so if I want some default post processing that requires access to the handlermethod, I have to add it to every controller I write and make sure everyone on my team knows to do this also?

If Jackson flushes the response, that makes sense. I wouldn't want to see or touch the response after Jackson writes it.
What would I do, use Jackson to read it back into java and try to infer the class? I want to manipulate the pojo before jackson distorts it (and likely removes a lot of information).

For interceptors to be useful in an ajax environment I think the required additional, or different, functionality for this bug request ought to be seriously considered.

[spring-projects-issues]

John Phelan commented

I want to wrap the returned pojo with jsonfilters based on annotations I apply to the controller method.
Long story short, this process also requires the interceptor to have access to the principal invoking the method, which apparently I cannot get.

[spring-projects-issues]

John Phelan commented

I ended up abandoning responseBody, which I feel is a shame. I added a HandlerMethodReturnValueHandler instead.

[spring-projects-issues]

Rossen Stoyanchev commented

A custom HandlerMethodReturnValueHandler is the appropriate mechanism to process controller return values.

[spring-projects-issues]

Graham Cox commented

I've just hit this exact problem myself, only from a different angle. I'm writing some code in Scala using Spring MVC - which works wonderfully, btw - and had planned on having my controller methods return Option[T] as the return type. I was then going to have an Interceptor that checks if this return type was None or Some[T], and in the case of None set the status code to 404.

This seems to me to be an ideal use of an interceptor, but as the interceptors are called after the response is committed they can't see the object that was returned and they can't change the status code of the response.

[spring-projects-issues]

Rossen Stoyanchev commented

Good to hear it's working well.

I don't think interceptors are the right mechanism for what you're trying to do. With @ResponseBody methods, the return value is written directly to the response using an HttpMessageConverter. A HanlderInterceptor, on the other hand is more commonly used with methods that rely on view resolution (e.g. ContentNegotiatingViewResolver) in which case the HandlerInterceptor gets to examine the selected view and the contents of the model. That's not necessarily the same as the return value, which could be either the view name or an attribute to be added to the model or even void or null. A custom HandlerMethodReturnValueHandler supporting return values of type Option might be a better fit.

[spring-projects-issues]

Graham Cox commented

I actually tried the HandlerMethodReturnValueHandler mechanism, but couldn't get Spring to call it at all. I'm not sure if I was configuring things incorrectly, or if it's some shortcoming between Spring and Scala, or what, but it just wouldn't ever get called.

In the end, I actually did it by using an Aspect to wrap all methods that had @RequestMapping on them and returned a type of Option[T]. The Jerkson based HttpMessageConverter that I'd written already correctly handled Option[T] to write out either the value or nothing, but it didn't do status code changes, and so this Aspect did that instead. I'm not happy with that solution, but it worked and got me moving on...

[spring-projects-issues]

Rossen Stoyanchev commented

With the resolution of #14728, HttpMessageConverter's no longer close the response. That still doesn't guarantee the response won't be committed (e.g. when the response buffer fills up) but at least ensures HttpMessageConverter's don't close the response.

/cc Phil Webb

[spring-projects-issues]

Marcel Overdijk commented

I'm also facing this issue. My scenario is different.
I'm using Spring as rest server where all controller actions return @ResponseBody annotated results.

Just like twitter I'm implementing 'suppress_response_codes' functionality.

Normally I will return json response like:

200: { "first_name": "Marcel", "last_name": "Overdijk" }

and in case of an error:

400: { "error": "the error message" }

When a user adds ?suppress_response_codes=true to the request the rest server will for each request return a 200, and it add the status code the response body:

200: { "status": 200, "data": { "first_name": "Marcel", "last_name": "Overdijk" } }
200: { "status": 400, "data": { "error": "the error message" } }

Is my best option to implement this in an ordinary servlet filter outside Spring? Or are there other options you can suggest?

/cc Rossen Stoyanchev

PS: I'm using the MappingJackson2HttpMessageConverter for json conversion.

[spring-projects-issues]

Marcel Overdijk commented

Thinking of this more I'm concerned using a servlet filter for this.
It would mean that I have to use a servlet response wrapper and have the original response as string; possible issue that I see is that when my response is big it (can) consume a lot of memory. I think this is also a good argument for re-opening this issue and incorporate intercepting of @RequestBody's properly into the framework.

Jersey 2.0 e.g. contains a ContainerResponseFilter where I can just easily retrieve the response entity before converting and writing it back to client. Note that in this the the response entity is just the original object I put in there, so it can any object.

[spring-projects-issues]

Rossen Stoyanchev commented

This ticket has a long history already and the original description does not match your request, so I'm closing this ticket. It would be better to open a new request with a suitable description.

[spring-projects-issues]

Marcel Overdijk commented

OK, I created ##15486

[spring-projects-issues]

Rossen Stoyanchev commented

thanks

[spring-projects-issues]

Rossen Stoyanchev commented

#15486 scheduled for 4.1 RC1 should provide an alternative solution through a new interception option. Please watch it if interested.