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
Special-case EmptyBody
in request encoder
#6752
Special-case EmptyBody
in request encoder
#6752
Conversation
@@ -139,7 +139,12 @@ private[ember] object Encoder { | |||
} | |||
} | |||
|
|||
if (!chunked && !appliedContentLength && !NoPayloadMethods.contains(req.method)) { | |||
if ( | |||
!appliedContentLength && req.body == EmptyBody && !NoPayloadMethods.contains(req.method) |
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.
should one maybe use reference equality eq
directly instead of equals(==)
?
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.
Good point, I'll make this change in #6753. We need to fix it in at least one other place.
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.
Should we do the same fix for blaze?
@@ -139,7 +139,12 @@ private[ember] object Encoder { | |||
} | |||
} | |||
|
|||
if (!chunked && !appliedContentLength && !NoPayloadMethods.contains(req.method)) { | |||
if ( | |||
!appliedContentLength && req.body == EmptyBody && !NoPayloadMethods.contains(req.method) |
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.
It'd be good to define def
s for those boolean expressions for better readability.
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.
I agree about readability but I feel superstitious about performance. Probably it will be inlined by JIT 🤔
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.
It will be nice when in scala 3 we write inline
and sleep easy.
I don't think blaze has this problem, based on the experiments in #4935. |
This PR replays #6444 for the ember request encoder. Specifically, when the body is known (by reference equality) to be the
EmptyBody
i.e. an emptyStream
, then aContent-Length: 0
header is added and chunked transfer encoding is disabled. This relates to #4935, and possibly even fixes it.