-
Notifications
You must be signed in to change notification settings - Fork 356
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
Blocked thread if WebApplicationException is reused. #4097 #5035
Conversation
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test for context.isCommitted()
may seem to be too much, we may only restrict the responses with entity (context.hasEntity() == true
). But then, the same issue can occur with for instance headers, when the header value is an inputstream for instance. Hence restricting all Responses seems like a proper way to go.
@@ -71,6 +71,7 @@ error.resources.cannot.merge=Resources do not have the same path and cannot be m | |||
error.request.abort.in.response.phase=The request cannot be aborted as it is already in the response processing phase. | |||
error.request.set.entity.stream.in.response.phase=The entity stream cannot be set in the request as it is already in the response processing phase. | |||
error.request.set.security.context.in.response.phase=The security context cannot be set in the request as it is already in the response processing phase. | |||
error.response.already.commited=Response was already committed. This response is being reused more than 1 time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response entity was already committed. The Response entity is being reused more than once.
Is it more explanatory to users? Maybe not that much correct, though.
|
||
@Override | ||
protected TestContainerFactory getTestContainerFactory() throws TestContainerException { | ||
return new JdkHttpServerTestContainerFactory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default TestContainerFactory seems to work as well.
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
Relates to #4097
To fix the blocked thread issue, it is only necessary the changes in class ResponseWriter
The other changes are related to fail with 500 when the response is reused more than 1 time.