finagle-http: Remove body (and Content-Length) from 1xx, 204 and 304 responses #521

monkey-mas · 2016-06-25T14:39:33Z

Problem

We currently have two problems. Firstly, as described in RFC2616 and
RFC7231, 204 and 304 responses must not contain a message-body. This
requirement was not enforced in finagle, i.e., a message-body can be
added. Moreover, a Content-Length header field can be added to these responses,
which is not allowed as mentioned in RFC7230#section-3.3.2.

We currently have two problems. Firstly, as described in RFC2616 and
RFC7230, 1xx, 204 and 304 responses must not contain a message-body. This
requirement was not enforced in finagle, i.e., a message-body can be
added. Moreover, a Content-Length header field can be added to 1xx and
204 responses, which is not allowed as mentioned in RFC7230#section-3.3.2.

Solution

First of all, we have to clear a message-body, which we call content in finagle,
if the response status code is either 204 or 304 to satisfy RFC7231. Secondly,
we need to make sure not to add a Content-Length header field in a 204 response
to follow RFC7230#section-3.3.2. With regard to a 304 response, a server may
send a Content-Length header as mentioned in the same section. Thus it'd be
better for us to allow users to set the field with a value of 0 if they like.
Most importantly, we make sure that these things are achieved and the new system
is bugwards-compatible by provding a new filter.

Firstly, we have to clear a message-body, which we call content in finagle,
if the response status code is either 1xx, 204 or 304 to satisfy RFC7230.
Then, we need to remove a Content-Length header field in 1xx and 204 responses
to follow RFC7230#section-3.3.2. With regard to a 304 response, we don't
intentionally remove it even though a Content-Length header field is set.

Result

We follow the specification of RFC with regard to a message-header and
message-body except that we have not added some header fields required in a
response with status code 304 to satisfy RFC7232#section-4.1; Any of the
following header fields must be generated: Cache-Control, Content-Location,
Date, ETag, Expires, and Vary.
As we achieve this by creating a new filter and set it to Http.scala, users
who are not willing to make use of the functionality can easily disable it
by removing the filter.

mosesn · 2016-06-25T19:19:28Z

Thanks for submitting the PR! We should make a decision about where we want to stick this behavior, and what we want finagle-http to enforce on behalf of its users.

I think that you're right that finagle should make it easy to follow the spec. With that said, we need to make sure it's at least bugwards migration compatible. For example, if your service is depending upon being able to do something that doesn't follow the spec, it would be nice to provide a clear migration to spec-compatibility for them. This might be as simple as making validation optional. Netty does this all the time, see validateHeaders

I think in the long-run we probably want to disallow constructing requests that are illegal, or configuring requests in an illegal way. This will improve the user experience, since you'll fail when you do the illegal thing, rather than when you send the bad object into the client. In the mean time, would it make sense to make request / response validation into a new filter? This way, if folks decide that they really can't live with this behavior, they can simply remove the filter themselves. It also separates concerns neatly, where the HttpServerDispatcher stays (mostly) just state machine stuff.

monkey-mas · 2016-06-26T16:03:55Z

Thanks a lot for your quick and detailed response!

With that said, we need to make sure it's at least bugwards migration compatible.
...
In the mean time, would it make sense to make request / response validation into a new filter?

Yes, I like your idea and strongly agree with it. :)

With regard to the actual implementation, we can 1) create a new filter and 2)add it to Http.scala like this so that finagle follows the spec by default and if some users don't like the filter they can just remove it? I might be wrong, please let me know if this is what you meant :)

mosesn · 2016-06-26T16:39:56Z

@monkey-mas 👍 that makes a lot of sense to me.

monkey-mas · 2016-06-27T14:46:44Z

Hi @mosesn,

I've rebased and updated my commit! Though I think it's ready to be reviewed, one test from finagle-mysql failed... Actually it passes on my local machine. If this is ok, please review my commits :)

mosesn · 2016-06-27T15:41:48Z

finagle-http/src/main/scala/com/twitter/finagle/http/codec/HttpServerDispatcher.scala

@@ -31,6 +31,11 @@ private[finagle] class HttpServerDispatcher(
    service.close()
  }

+  private def mayHaveContent(status: Status): Boolean = status match {


I don't think we need this anymore, since we have the new Filter now. What do you think?

AFAIU, (Http)ServerDispatcher#handle is called after all the filters are applied? In handle(), we add a Content-Length header field with a value of 0 in here. If I understand finagle's behaviour correctly, I think we can't avoid putting the code there?
(but I have to admit this is not nice though :(

I might be wrong, please give me your idea :)

I have the same question as @mosesn. What's the verdict here?

Hi @taylorleese cc: @mosesn ,
I might be wrong but it seems like handle(res: Reponse) is called multiple times and it's called again after all filters are applied (s.t., even after ComplianceFilter is applied) as I mentioned before. I found this when I debug my commit with sample code like this.

I think this means a Content-Length is added to a 204 response for example. Suppose we're going to return a 204 response;
1)we remove a Content-Length and message-body of a 204 response by using ComplianceFilter
2)but then handle is called and this line adds a Content-Length with a value of 0 to a 204 response as we've already removed a Content-Length in 1) and !rep.contentLength.isDefined holds.

To achieve what we want to do, i.e., send a 204 without a Content-Length and message-body for example, we need to have this condition here. The condition checks whether we send a Content-Length or not.

This is why I think we still need mayHaveContent. Anyway, I need your help to sort this out. Can you please help me? :)

Hi @monkey-mas, I'm new to this code so feel free to let me know if what follows is incorrect!
I don't think that handle(res: Response) is called multiple times: it's responsible for writing the message to the wire so if it's called multiple times we're in big trouble. I do think you're right in that its going to be called after all the filters so in its current form, its a problem.
It sounds like the problem is accommodating people sending a 204 with a body. In the case of people sending a 204 without a body, the content-length header gets set to 0. Maybe what we can do is have handle look for a 204 response with a content-length: 0 header, and strip those from the response? That will make 'legit' 204 responses legal by rfc 7230, and give your filter a way to signal that the header should be stripped.

To make this happen, the single if statement on 96 (old) might turn into:

rep.contentLength match { case Some(0) if rep.status == Status.NoContent => rep.headers.remove(Fields.ContentLength) case Some(_) => // NOOP: don't perturb explicitly set content lengths other than 204 with length 0. case None => rep.contentLength = rep.content.length }

There might be a cleaner way to implement that logic.

The filter would then set the content-length headers for Status(304), set it to 0 for Status(204), and strip away the bodies.

Hi @bryce-anderson,
Thanks for your advice! (and sorry for my late reply!) Well I think I don't fully follow your strategy, can you give me more details please?

I do think you're right in that its going to be called after all the filters so in its current form, its a problem.

Thanks for checking it, yeah that's what I expected :)

It sounds like the problem is accommodating people sending a 204 with a body

Do you mean we don't provide support for removing a body-message of 204 and 304 responses if a body is explicitly sent by users?

In the case of people sending a 204 without a body, the content-length header gets set to 0

Umm.. I think Content-Length header field is not set to 0 for a 204 response when there's no message-body. (But I think it's correct that the header field is added when we explicitly send a message-body, which is our problem here.)
More exactly, in the case of a 204 response with no body, rep.contentLength is None and rep.content.length is 0 I suppose.

Oh BTY, do you assume to use ComplianceFilter (or some filters) to handle the problem? It seems like you tried to solve the problem by just changing the original code at 96 in HttpServerDispatcher? (I thought so cos we don't have to check if a 204 response has contentLength=Some(0) after applying ComplianceFitler.)

TLDR: I've had some more time to think about it, and I'm not certain of an easy solution.

Umm.. I think Content-Length header field is not set to 0 for a 204 response when there's no message-body. (But I think it's correct that the header field is added when we explicitly send a message-body, which is our problem here.)
More exactly, in the case of a 204 response with no body, rep.contentLength is None and rep.content.length is 0 I suppose.

Sorry, its not clear to me if we agree. I believe at a Response with an empty body will get assigned a content length header of length 0. I haven't added tags to the handle function to check, but a raw Response should not be chunked and will not have a defined contentLength, so line 97 (on master, maybe this is the source of our confusion) should set the content-length to 0 on a 204 response without a body.

I don't think this can be solved only in the compliance Filter, there needs to be some coupling because, as is, the content-length is going to be set for non-chunked bodies regardless of status code. I personally would hate to make the HttpServerDispatcher aware of a single Filter, that just doesn't seem fair to the other filters. 😄 That suggests to me (and we should get @mosesn's opinion in light of what we've found) that this isn't actually a job for a filter. This feels more like the things that are getting delegated to the netty pipeline such as RespondToExpectContinue.

Sorry, its not clear to me if we agree. I believe at a Response with an empty body will get assigned a content length header of length 0. I haven't added tags to the handle function to check, but a raw Response should not be chunked and will not have a defined contentLength, so line 97 (on master, maybe this is the source of our confusion) should set the content-length to 0 on a 204 response without a body.

Yes, I understand the code in the same way. Thanks for your explanation! :) (I was thinking about Content-Length of rep where the code on line 97 is not applied.)

That suggests to me (and we should get @mosesn's opinion in light of what we've found) that this isn't actually a job for a filter. This feels more like the things that are getting delegated to the netty pipeline such as RespondToExpectContinue.

Okay, so you mean we create a ChannelHandler, like RespondToExpectContinue, instead of using a Filter and give it a responsibility to handle 204 and 304 responses? Correct? Honestly, I'm not familiar with Netty's pipeline and not quite sure how it works in finagle. What sort of high-level solution (or actual implementation) do you have in your mind? (Can you please be more specific?)

I would definately wayt for @mosesn to chime in before going down the netty pipeline route. As a preview, I'll give my understanding of it. The code at https://github.com/twitter/finagle/blob/develop/finagle-http/src/main/scala/com/twitter/finagle/http/Codec.scala#L307 gives an example of how the RespondToExpectContinue handler works, but it is an UpstreamHandler (I believe that corresponds to InboundHandler in netty4). The DownstreamHandler works essentially the same but in the other direction. In my minds eye, the DownstreamHandler intercepts the HttpResponse and drops the body and requisite headers.

One thing that occurs to me is that streaming responses (chunked) don't appear to be addressed in this commit. The way streaming responses are rendered appears more complex and might make using the netty pipeline more tricky, if even possible. For example, In the case of a 304 or 204 I doubt that we want to use the Reader, and instead we would want to discard it. I don't think that is a possibility, at least in the place I pointed you to above.

mosesn · 2016-06-27T15:50:01Z

LGTM at a high level. Can you also add a note to finagle/CHANGES? I think this is worth calling out for our users.

Problem We currently have two problems. Firstly, as described in RFC2616[1] and RFC7231[2], 204 and 304 responses must not contain a message-body. This requirement was not enforced in finagle, i.e., a message-body can be added. Moreover, a Content-Length header field can be added to these responses, which is not allowed as mentioned in RFC7230#section-3.3.2[3]. Solution First of all, we have to clear a message-body, which we call content in finagle, if the response status code is either 204 or 304 to satisfy RFC7231. Secondly, we need to make sure not to add a Content-Length header field in a 204 response to follow RFC7230#section-3.3.2. With regard to a 304 response, a server may send a Content-Length header as mentioned in the same section. Thus it'd be better for us to allow users to set the field with a value of 0 if they like. Most importantly, we make sure that these things are achieved and the new system is bugwards-compatible by provding a new filter.[4] Result We follow the specification of RFC with regard to a message-header and message-body except that we have not added some header fields required in a response with status code 304 to satisfy RFC7232#section-4.1[5]; Any of the following header fields must be generated: Cache-Control, Content-Location, Date, ETag, Expires, and Vary. As we achieve this by creating a new filter and set it to Http.scala[6], users who are not willing to make use of the functionality can easily disable it by removing the filter. [1] https://tools.ietf.org/html/rfc2616 [2] https://tools.ietf.org/html/rfc7231 [3] https://tools.ietf.org/html/rfc7230#section-3.3.2 [4] twitter#521 (comment) [5] https://tools.ietf.org/html/rfc7232#section-4.1 [6] https://github.com/monkey-mas/finagle/blob/finagle-6.35.0/finagle-http/src/main/scala/com/twitter/finagle/Http.scala#L298-L304

monkey-mas · 2016-06-28T15:30:06Z

Thanks for the review! Except this point, I think the fix commit should be fine. :) I'll squash additional commits into one after everything is ok.

Looking forward to your feedback! 😀

monkey-mas · 2016-06-30T14:41:16Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/ComplianceFilter.scala

+          logger.warning(s"Response with a status code of ${rep.statusCode} must not have a Content-Length header field thus the field has been removed.")
+        }
+
+        //Make sure a Content-Length header field is set with a value of 0 if it is explicitly sent for a 304 response


Oops, I think I was wrong. As RFC 7230 section3.3.2 says, if we do want to send a Content-Length header field, we should return the Content-Length value of the cached content which has been returned to a client in a 200 response. I thought a value of Content-Length header field was always 0 as the content of a 304 response is always empty...
The following is the citation from the section;
A server MAY send a Content-Length header field in a 304 (Not Modified) response to a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a 200 (OK) response to the same request.

Oof, this doesn't feel like it's easy to know. I guess this is something that users must set? I guess it's better to just not modify the content length in this case.

I guess this is something that users must set?

I think so if users really want to send the header field in a 304 response... I'm not sure how to validate that they've chosen the correct Content-Length value. As the RFC says, if the field-value doesn't equal the decimal number of octets that would have been sent in the payload body of a 200 (OK) response to the same request then we MUST NOT send Content-Length.

I guess it's better to just not modify the content length in this case.

Yeah, not modifying is better I suppose so. Will fix it :)

I feel that things will be more complicated if we start to think about a Content-Length value of a 304 response as I've mentioned. Do you think we could stick to the original problems we had before and hopefully we can find a way to handle the new issue we've found now? What do you think?

Fixed it :)

mosesn · 2016-07-08T17:42:03Z

@monkey-mas sorry for the delay, haven't had a chance to take a look this week. but I haven't forgotten about you!

monkey-mas · 2016-07-09T02:40:04Z

@mosesn haha, sure sure. :) Please have a look when you have time and let me know then!

bryce-anderson · 2016-08-15T22:10:59Z

Okay @monkey-mas, I have discussed this with @mosesn and there may be another way to get this done in a bugwards compatible way. A lot of the functionality in the HttpServerDispatcher.handle method deals with similar issues to what you're addressing, namely ensuring that the Response is in a conformant state. Perhaps this isn't something that the dispatcher should be concerned with and this could be moved from the HttpServerDispatcher to your ComplianceFilter.
This is bugwards compatible in that those who need the older behavior could replace the filter you've made.

What do you think?

monkey-mas · 2016-08-16T16:35:52Z

@bryce-anderson, cc: @mosesn
Thanks for having a discussion! Yes, I think it sounds nice! :)

Perhaps this isn't something that the dispatcher should be concerned with and this could be moved from the HttpServerDispatcher to your ComplianceFilter.

Just to make sure, do you mean we leave handle's core functionality and move rest of the functionality to ComplianceFilter?

If so, I think we need to make two things clear:

which functionality can we move to ComplianceFilter?
If I understand your thought correctly, we might move:
1. setKeepAlive
2. others: e.g., removeContentLength, setTransferEncoding and addContentLength
  
  It could be not really good to move these functionality if we take the comment into account though???
can we safely achieve it(moving functionality to the filter) and maintain the current finagle's behaviour?
I think the biggest reason why we've added the problematic logic to handle is to make sure we apply the changes, such as removing a Content-Length and so on, just before we actually dispatch a response (after applying all filters and so forth)?
Based on your idea, ComplianceFilter will take this responsibility? If we can't guarantee that ComplianceFilter is used (just) before handle is called, then we might change finagle's behaviour? I might be wrong but I mean it's not nice if there's a possibility that another filter or functionality, which adds a Content-Length for example, can be called after ComplianceFilter is used to filter a response? (or is this fine?)

What do you think?

mosesn · 2016-08-16T17:06:05Z

@monkey-mas good catch with that comment. maybe the first step is to fix netty's content compressor? then we can make the rest of these changes more safely.

bryce-anderson · 2016-10-25T21:26:40Z

@monkey-mas, I'm sorry we've been slow on this. We have started to move in the direction needed to incorporate your commit. See fa8cef1
If you're interested, perhaps we can get your work into the ResponseConformanceFilter.

- fix not to use ChannelBuffers.EMPTY_BUFFER - handle 1xx status code condition by checking 100 <= status < 200 - fix cols <= 100 - add payload size to error log

- remove netty dependency - move test(...) into a loop

- remove unnecessary end-to-end test cases - move testIfImplemented(...) in a loop - fix cols <= 100

mosesn · 2017-05-09T21:30:22Z

@monkey-mas sorry, we dropped the ball on this! I'm going to see if I can get this over the goalpost and merge it in. Will keep you posted!

mosesn · 2017-05-10T00:44:20Z

this is failing in really strange ways. I think the messiness of finagle's HTTP compliance stuff is catching up to us. I'm going to keep on trying to make it work, but might have to give up if it takes too much time. If you have some time to look at this again, I'd be happy to send you what I've done so far.

monkey-mas · 2017-05-10T15:06:38Z

@mosesn
My apologies. Sorry for not handling this PR for a long time.

this is failing in really strange ways. I think the messiness of finagle's HTTP compliance stuff is catching up to us. I'm going to keep on trying to make it work, but might have to give up if it takes too much time. If you have some time to look at this again, I'd be happy to send you what I've done so far.

Honestly, I've got no idea why it fails. You have some clue, don't you?

I think I can have some time to fix it. Can you please give me some information you've collected so far?
If things go wrong (or if you'd think it's not good to keep doing this stuff), let's close this PR. (Well, that's my fault for keeping the PR open... Sorry again! :))

mosesn · 2017-05-10T18:00:59Z

@monkey-mas I ended up undoing most of my changes since they didn't seem to be helping. I'm not sure where the issue is, but I've merged against master and made a PR: monkey-mas#1

monkey-mas · 2017-05-12T17:00:14Z

@mosesn Thanks so much! Let me have a look at it :)

merge master

monkey-mas · 2017-05-21T18:08:45Z

@mosesn @bryce-anderson
Okay I kinda understand our situation. I might be wrong but give me your thoughts please :)

Problem

The current (wierd) problem we have is our response gives us Content-Length: 0 even if we remove it using ResponseConformanceFilter
Actually end-to-end test fails like:

[info] - response with status code 204 must not have a message body nor Content-Length header field when non-empty body with explicit Content-Length is returned *** FAILED ***
[info]   Some(0) was not empty (AbstractEndToEndTest.scala:1472)

Based on the fact that the unit test ResponseConformanceFilterTest passes, I think the filter works properly.

What I've observed so far

I've done some print-debugging (& stack tracing) and found our code works as follows:

Body & Content-Length are set in test code
Netty3HeaderMap#set(Content-Length , 9) is called
Body & Content-Length are both removed using ResponseConformanceFilter
Bijections#fullResponseToFinagle(res) and Netty3HeaderMap#add(content-length, 0) are called where res is

DefaultFullHttpResponse(decodeResult: success, version: HTTP/1.1, content: EmptyByteBufBE)
HTTP/1.1 204 No Content
content-length: 0)

Additionally, I roughly checked if our code adds the header somewhere, by using git grep 'contentLength' for example, but it seems not.

Based on this, I think Content-Length: 0 is set when fullResponseToFinagle(...) is called, which ends up with contentLength == Some(0) thus test fails. It could be possible that Netty adds Content-Length: 0 and we have the header in NettyHttp.FullHttpResponse when calling fullResponseToFinagle(...)?

(Naive) Solution

With that said I'm still not 100% for sure how and when Content-Length: 0 is set. If my guess is correct, the header is added after filters are applied to a response. So we can't avoid handling the issue within Bijections like below?

@@ -88,15 +90,21 @@ private[finagle] object Bijections {
     }
 
     def fullResponseToFinagle(rep: NettyHttp.FullHttpResponse): FinagleHttp.Response = {
       val payload = ByteBufAsBuf(rep.content)
+      val status = rep.status
 
       val resp = FinagleHttp.Response(
         versionToFinagle(rep.protocolVersion),
-        statusToFinagle(rep.status),
+        statusToFinagle(status),
         BufReader(payload)
       )
 
       resp.setChunked(false)
+      if (status.codeClass() == HttpStatusClass.INFORMATIONAL || status == HttpResponseStatus.NO_CONTENT) {
+        rep.headers().remove("Content-Length")
+      }
       writeNettyHeadersToFinagle(rep.headers, resp.headerMap)
       resp.content = payload

mosesn · 2017-05-22T17:59:18Z

Good sleuthing! I would love to know why content length is being set to 0, but I'm OK with pulling this in before we know. @bryce-anderson what do you think?

monkey-mas · 2017-05-24T18:51:29Z

Finally I determined the cause of Content-Length: 0. I believe it's set in HttpObjectAggregator#finishAggregation(...) by netty.

The following is the rough sketch of (reversed) execution paths to encounter Content-Length: 0.

3. Bijections#fullResponseToFinagle(...)

2. UnpoolHttpHandler#channelRead(...)

1. HttpObjectAggregator#finishAggregation(...)

As I've mentioned before, the header's already set to 0 when Bijections#fullResponseToFinagle(...) is executed. Before this, UnpoolHttpHandler#channelRead(ctx: ChannelHandlerContext, msg: Any) is called where msg is something like

HttpObjectAggregator$AggregatedFullHttpResponse(decodeResult: success, version: HTTP/1.1, content: CompositeByteBuf(ridx: 0, widx: 0, cap: 0, components=0))
HTTP/1.1 204 No Content
content-length: 0

As msg indicates, we've already got content-length: 0. This header is actually appended when HttpObjectAggregator#finishAggregation(FullHttpMessage aggregated) finishes execution. Because we've already removed Content-Length header using ResponseConformanceFilter, aggregated(AggregatedFullHttpResponse) doesn't have the corresponding header at the beginning of the method call. Actually aggregated.message.headers is DefaultHeadersImpl[] (empty). This leads to our unexpected result, i.e., Content-Length is set to String.valueOf(aggregated.content().readableBytes())) where

aggregated.content() == CompositeByteBuf(ridx: 0, widx: 0, cap: 0, components=0)
aggregated.content().readableBytes == (this.writerIndex - this.readerIndex) == 0

ref. readableBytes

Therefore, aggregated has Content-Length: 0 here.

bryce-anderson · 2017-05-25T23:02:29Z

I'll pull it in and take another look! Sounds like this is further down in the pipeline a bit, so this is more interesting than we'd hoped. :) Excellent sleuthing @monkey-mas.

bryce-anderson · 2017-05-30T20:47:12Z

@monkey-mas, I've looked through your PR and wanted to get your take: it does look like Netty is responsible for the test failures as you've laid out above. How do you want to proceed? In my mind's eye, Netty should probably be fixed, so if you're interested in contributing to that project I think you've got a nice contribution on deck. In the meantime, we could disable the end-to-end tests that are failing due to the client issue.

What do you think?

monkey-mas · 2017-06-05T10:59:43Z

@bryce-anderson Sorry for my late reply.

In the meantime, we could disable the end-to-end tests that are failing due to the client issue.

Do you mean we can just merge functionality of handleNoMessageResponse to ResponseConformanceFilter without end-to-end tests? Or should we lock this PR until Netty solves the problem? Either way sounds good to me, let me know your opinion :)

In my mind's eye, Netty should probably be fixed, so if you're interested in contributing to that project I think you've got a nice contribution on deck.

Cool! I'll open a PR related to this issue though I'm not quite sure if Netty community likes the change to follow RFC7230. :(
Anyway I'll keep you updated! :)

bryce-anderson · 2017-06-05T14:43:17Z

@monkey-mas, no worries! I'm in the process of merging this in with the end-to-end tests disabled: this works as expected when running a simple telnet session.

I feel the Netty folks are generally quite receptive to patches that make sure they don't break things, which this seems to be an instance of. They already have similar logic laying around to deal with cases where a body shouldn't exist, for example here. If you ask me, that code would be better off renamed and in the HttpUtil object.

bryce-anderson · 2017-06-06T17:52:12Z

@monkey-mas, after long last, this was merged as commit 1e4252e. Thanks a lot for both your patience and good work! Keep us updated on the status of any Netty PR you submit to address the chunk aggregation issue.

monkey-mas · 2017-06-07T16:01:32Z

@bryce-anderson @mosesn
Actually thanks a lot for your patience and great feedback! It wouldn't be possible without your support :)

Okay, I'll get started with the aggregation issue. The easiest solution could be to modify the logic of finishAggregation(...) as this should be the last point before an actual response is returned to a client.
Moreover, some methods can be moved to HttpUtil and refactored like you said. Anyway let's discuss it at Netty's github :)

bryce-anderson · 2017-06-07T16:04:40Z

@monkey-mas 👍
Please feel free to ping me in any related Netty issue/PR you file.

vkostyukov · 2017-06-21T00:14:08Z

Hey @monkey-mas! We had to revert this temporary (see b5cbb3e) for some internal reasons. We'll put it back as soon as we can. Stay tuned and thanks again - this is a solid piece of work!

monkey-mas · 2017-06-21T16:25:12Z

Oops sorry to hear that :( I hope all is well at your end.
(Anyway thanks for kindly letting me know!)

vkostyukov · 2017-07-07T21:58:41Z

Hi @monkey-mas! I'm trying to put your work back where it belongs. Will update this thread when it's merged in.

vkostyukov · 2017-07-10T22:36:28Z

It's merged (see f8208cd)!

monkey-mas force-pushed the fix-body-and-header-in-204-and-304 branch from 6cbea75 to 5a983fa Compare June 27, 2016 06:48

monkey-mas force-pushed the fix-body-and-header-in-204-and-304 branch from 5a983fa to 91919ad Compare June 27, 2016 06:57

monkey-mas force-pushed the fix-body-and-header-in-204-and-304 branch from 91919ad to 656d289 Compare June 27, 2016 07:03

mosesn reviewed Jun 27, 2016
View reviewed changes

monkey-mas force-pushed the fix-body-and-header-in-204-and-304 branch from e2a4b9a to f0856db Compare June 28, 2016 14:58

monkey-mas force-pushed the fix-body-and-header-in-204-and-304 branch from f0856db to 40f9b2e Compare June 29, 2016 13:43

monkey-mas reviewed Jun 30, 2016
View reviewed changes

bryce-anderson mentioned this pull request Oct 24, 2016

connection is not kept alive when receiving 204-no-content without content-length #563

Closed

monkey-mas added 3 commits November 2, 2016 00:32

[review fix] Fix ResponseConformanceFilter

37a41f7

- fix not to use ChannelBuffers.EMPTY_BUFFER - handle 1xx status code condition by checking 100 <= status < 200 - fix cols <= 100 - add payload size to error log

[review fix] Fix ResponseConformanceFilterTest

865e018

- remove netty dependency - move test(...) into a loop

[review fix] Fix EndToEndTest

057ed65

- remove unnecessary end-to-end test cases - move testIfImplemented(...) in a loop - fix cols <= 100

mosesn added the Ship It label May 9, 2017

Merge branch 'develop' into fix-body-and-header-in-204-and-304

1e8497e

Merge pull request #1 from mosesn/fix-body-and-header-in-204-and-304

2dfc051

merge master

mosesn added In Progress and removed Ship It labels May 17, 2017

bryce-anderson added the Ship It label Jun 5, 2017

bryce-anderson closed this Jun 6, 2017

monkey-mas deleted the fix-body-and-header-in-204-and-304 branch June 7, 2017 16:01

finagle-http: Remove body (and Content-Length) from 1xx, 204 and 304 responses #521

finagle-http: Remove body (and Content-Length) from 1xx, 204 and 304 responses #521

Conversation

monkey-mas commented Jun 25, 2016 • edited

mosesn commented Jun 25, 2016

monkey-mas commented Jun 26, 2016

mosesn commented Jun 26, 2016

monkey-mas commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monkey-mas Jul 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosesn commented Jun 27, 2016

monkey-mas commented Jun 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosesn commented Jul 8, 2016

monkey-mas commented Jul 9, 2016

bryce-anderson commented Aug 15, 2016

monkey-mas commented Aug 16, 2016

mosesn commented Aug 16, 2016

bryce-anderson commented Oct 25, 2016

mosesn commented May 9, 2017

mosesn commented May 10, 2017

monkey-mas commented May 10, 2017

mosesn commented May 10, 2017

monkey-mas commented May 12, 2017

monkey-mas commented May 21, 2017

Problem

What I've observed so far

(Naive) Solution

mosesn commented May 22, 2017

monkey-mas commented May 24, 2017

bryce-anderson commented May 25, 2017

bryce-anderson commented May 30, 2017

monkey-mas commented Jun 5, 2017

bryce-anderson commented Jun 5, 2017

bryce-anderson commented Jun 6, 2017

monkey-mas commented Jun 7, 2017

bryce-anderson commented Jun 7, 2017

vkostyukov commented Jun 21, 2017

monkey-mas commented Jun 21, 2017

vkostyukov commented Jul 7, 2017

vkostyukov commented Jul 10, 2017

monkey-mas commented Jun 25, 2016 •

edited

monkey-mas Jul 6, 2016 •

edited

monkey-mas commented Jun 28, 2016 •

edited