getOutputStream() has already been called for this response - still not completely fixed #155
Assignees
Labels
Comments
|
I cloned your repo and tried to reproduce it, but if I start it with and in a second terminal I perform requests (in this example using httpie): I see no exception but the following output: I can definitely see that we don't handle the error dispatch correctly, since there are 6 log messages but I'd have expected 2. |
|
Sorry I forgot to mention that same as in #146, the sample needs to be deployed to Tomcat. Anyway I think what you mentioned about incorrect error handling when running as Spring Boot app might have same cause. |
|
From what I can see the problem is in
This could be reproduced by flushing response: diff --git a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/SecurityStrategy.java b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/SecurityStrategy.java
index f901357..a725a84 100644
--- a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/SecurityStrategy.java
+++ b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/SecurityStrategy.java
@@ -41,6 +41,7 @@ final class SecurityStrategy implements Strategy {
}
if (correlator.isPresent()) {
+ response.getWriter().flush();
correlator.get().write(response);
}
}
As far as I understand design, Logbook should be an most outer filter (@whiskeysierra is it correct?) but |
This happens because we apply filters to body while logging thus effectively using body's getOutputStream |
|
Looks like we have to collect nested chain output in LocalResponse buffer (what we already do), filter and log it, and only then copy this buffer to real http response output stream. |
|
In such case we can see whether buffer is empty, i.e. nested chain did not produce anything and can avoid calling real http response's getOutputStream so ErrorPageHandler can take care of it. Downside is that error page itself will not be logged by logbook |
|
@davidjirovec How do you quickly deploy your sample project in a standalone tomcat? I tried But that didn't seem to deploy the Spring Application correctly. |
|
I configured the |
|
HTTP/1.1 401 Unauthorized
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Content-Type: application/json;charset=UTF-8
Date: Wed, 22 Nov 2017 14:00:39 GMT
Expires: 0
Pragma: no-cache
Server: Apache-Coyote/1.1
Strict-Transport-Security: max-age=31536000 ; includeSubDomains
Transfer-Encoding: chunked
WWW-Authenticate: Basic realm="Spring"
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
X-XSS-Protection: 1; mode=block
{
"error": "Unauthorized",
"message": "Full authentication is required to access this resource",
"path": "/demo/foo",
"status": 401,
"timestamp": 1511359239651
}Log output is: |
|
@davidjirovec I tried your project but I can't reproduce the issue. Can you help? |
|
I hope I will find time to help you this week. |
|
Ok, I see you are trying Wait for deployment to finish, then try to access
|
It's caused by (or originates from) the |
|
I don't remember why we register the filter for the error dispatch though. We might stop doing that. Should resolve the issue as well. |
|
I'd consider this to be a bug in Spring. Current workaround is to: @EnableAutoConfiguration(exclude = ErrorMvcAutoConfiguration.class)Alternatively, you'd need to wait for Spring Boot 2, since they include a fix. See spring-projects/spring-boot#11580. I'll close this for now. Feel free to re-open/challenge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Same problem as #146. Although #146 was fixed, problem is back when I turn on logbook request response logging by adding
to application.properties.
Minimal example to reproduce is still prepared here. Revision d46137b contains updated logbook version and the mentioned logging configuration.
The text was updated successfully, but these errors were encountered: