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

For 204 response the body should be empty... #321

Closed
wants to merge 2 commits into from

Conversation

stoyle
Copy link

@stoyle stoyle commented May 20, 2017

...not an empty ByteArrayInputStream.

When proxying through the server a body here will result in a
Transfer-Encoding:chunked in the aleph server, basically stating the body size
is unknown. Some caching products, like varnish, fails miserably here, e.g. gives
the calling client back an http 503.

With this change, at least when the body is empty it is removed, and the
Transfer-Encoding:chunked is not added.

This seems to be working. I guess there is a deeper root cause here, somewhere in Netty perhaps? I would be happy to change this in any way you'd like, or attack the problem differently. Let me know.

Cheers,
Alf

stoyle added 2 commits May 20, 2017 13:44
…tStream.

When proxying through the server a body here will result in a
`Transfer-Encoding:chunked` in the aleph server, basically stating the body size
is unknown. Some caching products, like varnish, fails miserably here, e.g. gives
the calling client back an http 503.

With this change, at least when the body is empty it is removed, and the
`Transfer-Encoding:chunked` is not added.
To avoid the reflection warning in the build.
@danielcompton
Copy link
Member

I'd be interested in hearing @malcolmsparks thoughts on this, as Yada tries very hard to handle HTTP semantics well. I think Yada only sets a 204 on yada.methods when there isn't a body so it wouldn't hit this?

@stoyle
Copy link
Author

stoyle commented May 21, 2017

I am not too familiar with Yada, but if you are wrapping both aleph's client and server, and haven't done anything to prevent this, I think you will have the same issue.

I guess this is a small issue, but it was very confusing when I ran into it. If others run into it in the future, there is at least a solution on the alephs mailing list (https://groups.google.com/forum/#!topic/aleph-lib/00pJj6s_UE0).

@ztellman
Copy link
Collaborator

This is only a problem when you're proxying through a 204 response from Aleph's HTTP client, I don't think it should affect Yada much.

However, this fix assumes the body is always an InputStream, which won't be true when raw-stream? is true. It may be better to fix this a little further upstream, I'm happy to take care of it myself if you're not comfortable digging in there.

@danielcompton
Copy link
Member

Ah right, I completely missed that this is a client 204, not a server 204 response, sorry.

@stoyle
Copy link
Author

stoyle commented May 22, 2017

Go ahead, looking forward to seeing how to fix it properly. It was fun digging a bit in Aleph's code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants