Carefully manage Keep-Alive/Close connection headers in all examples#8966
Carefully manage Keep-Alive/Close connection headers in all examples#8966normanmaurer merged 1 commit intonetty:4.1from
Conversation
|
Can one of the admins verify this patch? |
|
@netty-bot test this please |
There was a problem hiding this comment.
@kachayev one question... isn't this assuming that we are talking HTTP/1.1 ? I mean for 1.0 we would need to add the keep-alive imho
There was a problem hiding this comment.
Yeah, probably you're correct: we're responding with 1.1 but any 1.0 client would not be able to process this correctly.
There was a problem hiding this comment.
@normanmaurer Done! HttpStaticFileServerHandler was not that simple to rework because of exception catch branch. I also updated HttpUploadServerHandler to use HttpUtils. PTAL.
There was a problem hiding this comment.
@normanmaurer Another question here, is it generally safe to close the connection after "write" operation instead of after "flush"?
There was a problem hiding this comment.
yes as the write is only be "executed" when the flush happens. Executed here means that it is "written to the socket".
There was a problem hiding this comment.
Okay, cool! I'm checking just because this is the only place we're adding the listener to .write when in other cases to .writeAndFlush.
d076863 to
9891f35
Compare
There was a problem hiding this comment.
why ? It should work also with pipelining atm.
There was a problem hiding this comment.
The request is reassigned on each channelRead0. If we read, let's say, 3 requests consistently and the exception has occurred when writing the 2nd response, we still rely on the properties of the last request. I'm not sure if it's possible to catch in this particular scenario as the processing of each request is pretty streamlined.
There was a problem hiding this comment.
got it... I think thats fine.
There was a problem hiding this comment.
Yeah, I think that works too. Do you think it's better to remove the comment to avoid confusion?
|
Yep
… Am 02.04.2019 um 06:47 schrieb Oleksii Kachaiev ***@***.***>:
@kachayev commented on this pull request.
In example/src/main/java/io/netty/example/http/file/HttpStaticFileServerHandler.java:
> @@ -110,37 +110,42 @@
public static final String HTTP_DATE_GMT_TIMEZONE = "GMT";
public static final int HTTP_CACHE_SECONDS = 60;
+ // Note, that this approach for storing an instance of "current" request works
+ // only in case we don't need to support requests pipelining.
+ private FullHttpRequest request;
Yeah, I think that works too. Do you think it's better to remove the comment to avoid confusion?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Motivation: "Connection: close" header should be specified each time we're going to close an underlying TCP connection when sending HTTP/1.1 reply. Modifications: Introduces changes made in netty#8914 for the following examples: * WebSocket index page and WebSocket server handler * HelloWorld server * SPDY server handler * HTTP/1.1 server handler from HTTP/2 HelloWorld example * HTTP/1.1 server handler from tiles example Result: Keep-Alive connections management conforms with RFCs.
9891f35 to
f498f7d
Compare
|
@normanmaurer Done! |
|
@netty-bot test this please |
…8966) Motivation: "Connection: close" header should be specified each time we're going to close an underlying TCP connection when sending HTTP/1.1 reply. Modifications: Introduces changes made in #8914 for the following examples: * WebSocket index page and WebSocket server handler * HelloWorld server * SPDY server handler * HTTP/1.1 server handler from HTTP/2 HelloWorld example * HTTP/1.1 server handler from tiles example Result: Keep-Alive connections management conforms with RFCs.
|
@kachayev merged thanks |
Motivation:
"Connection: close" header should be specified each time we're going to close an underlying TCP connection when sending HTTP/1.1 reply.
Modifications:
Introduces changes made in #8914 into the following examples:
Result:
Keep-Alive connections management conforms with RFCs.