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

Carefully manage Keep-Alive/Close connection headers in all examples #8966

Merged
merged 1 commit into from Apr 2, 2019

Conversation

@kachayev
Copy link
Contributor

kachayev commented Mar 21, 2019

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:

  • 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.

@netty-bot

This comment has been minimized.

Copy link

netty-bot commented Mar 21, 2019

Can one of the admins verify this patch?

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 22, 2019

@netty-bot test this please

ctx.write(response).addListener(ChannelFutureListener.CLOSE);
} else {
response.headers().set(CONNECTION, KEEP_ALIVE);
if (keepAlive) {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Mar 28, 2019

Member

@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

This comment has been minimized.

Copy link
@kachayev

kachayev Mar 29, 2019

Author Contributor

Yeah, probably you're correct: we're responding with 1.1 but any 1.0 client would not be able to process this correctly.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Mar 29, 2019

Member

Mind fixing it ?

This comment has been minimized.

Copy link
@kachayev

kachayev Mar 29, 2019

Author Contributor

Sure, will do.

This comment has been minimized.

Copy link
@kachayev

kachayev Mar 31, 2019

Author Contributor

@normanmaurer Done! HttpStaticFileServerHandler was not that simple to rework because of exception catch branch. I also updated HttpUploadServerHandler to use HttpUtils. PTAL.

} else {
// Tell the client we're going to close the connection.
response.headers().set(CONNECTION, CLOSE);
ctx.write(response).addListener(ChannelFutureListener.CLOSE);

This comment has been minimized.

Copy link
@kachayev

kachayev Mar 30, 2019

Author Contributor

@normanmaurer Another question here, is it generally safe to close the connection after "write" operation instead of after "flush"?

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Apr 1, 2019

Member

yes as the write is only be "executed" when the flush happens. Executed here means that it is "written to the socket".

This comment has been minimized.

Copy link
@kachayev

kachayev Apr 1, 2019

Author Contributor

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.

@kachayev kachayev force-pushed the kachayev:fix-keep-alive-examples branch from d076863 to 9891f35 Mar 31, 2019
@@ -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;

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Apr 1, 2019

Member

why ? It should work also with pipelining atm.

This comment has been minimized.

Copy link
@kachayev

kachayev Apr 1, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Apr 1, 2019

Member

got it... I think thats fine.

This comment has been minimized.

Copy link
@kachayev

kachayev Apr 2, 2019

Author Contributor

Yeah, I think that works too. Do you think it's better to remove the comment to avoid confusion?

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 2, 2019

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 kachayev force-pushed the kachayev:fix-keep-alive-examples branch from 9891f35 to f498f7d Apr 2, 2019
@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Apr 2, 2019

@normanmaurer normanmaurer added this to the 4.1.35.Final milestone Apr 2, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 2, 2019

@netty-bot test this please

@normanmaurer normanmaurer merged commit 5241123 into netty:4.1 Apr 2, 2019
3 checks passed
3 checks passed
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
normanmaurer added a commit that referenced this pull request Apr 2, 2019
…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.
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 2, 2019

@kachayev merged thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.