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

Allow sending Connection: close header #4531

Merged
merged 17 commits into from
Mar 8, 2023

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Nov 11, 2022

Motivation:

This PR aims to solve two problems with Connection header.

  1. Allow sending Connection header field.

    HTTP/2 does not use the Connection header field.
    https://httpwg.org/specs/rfc9113.html#rfc.section.8.2.2
    Hence, Connection header is prohibited and it is automatically stripped from headers.

    There have been requests to close a connection after sending requests or
    responses on purpose or send Connection: close header for
    compatibility with legacy HTTP/1 servers.

    For HTTP/2, we can translate Connection: close as a signal to close
    a connection by sending a GOAWAY frame.
    It would be useful to rebalance HTTP/2 loads by closing
    connections after some periods.

  2. Armeria server does not return Connection: close header when Connection: close is received.

    As a client sent Connection: close header, it seems fine not to
    return Connection: close header. However, returning the
    Connection: close header would be more compliant with HTTP/1.1
    protocol.

Modifications:

  • Removed CONNECTION header from prohibited header names.
  • Added KeepAliveHandler.disconnectWhenFinished() which is called when a Connection: close is specified in response headers.
  • Modified HttpServerHandler to check whether to close a connection when responses have been written.
  • Removed the singleton instance of NoopKeepAliveHandler and make to create an instance for each connection.
    • NoopKeepAliveHandler now has two fields to know whether a channel needs immediate disconnection or the connection has to be closed after receiving all responses in process.
  • Fixed HttpChannelPool to determine if a session is healthy using isActive(). This change allows in flights requests to use HTTP/2 sessions before GOAWAY is sent or received.
  • Fixed to correctly set keep-alive headers for HTTP/1.1.

Result:

Motivation:

This PR aims to solve two problems with `Connection` header.

1. Allow sending `Connection` header field.

  HTTP/2 does not use the `Connection` header field.
  https://httpwg.org/specs/rfc9113.html#rfc.section.8.2.2
  Hence, `Connection` header is prohibited and it is automatically
  stripped from headers.
  https://github.com/line/armeria/blob/b127cd27252f6a454130f66d1175a06faed01f09/core/src/main/java/com/linecorp/armeria/client/ClientOptions.java#L132-L132

  There have been requets to close a connection after sending requests or
  responses on purpose or send `Connection: close` header for
  compatibility with legacy HTTP/1 servers.

  For HTTP/2, we can translate `Connection: close` as a signal to close
  a connection by sending a GOAWAY frame.
  It would be useful to rebalance HTTP/2 loads by closing
  connections after some periods.

2. Armeria server does not return `Connection:close` header when
   `Connection: close` is received.

  As a client sent `Connection: close` header, it seems fine not to
  return `Connection: close` header. However, returning the
  `Connection: close` header would be more compliant with HTTP/1.1
  protocol.

Modifications:

- Removed `CONNECTION` header from prohibited header names.
- Added `KeepAliveHandler.disconnectWhenFinished()` which is called when
  a `Connection: close` is specified in response headers.
- Modified `HttpServerHandler` to check whether to close a connection
  when responses have been written.
- Removed the singleton instance of `NoopKeepAliveHandler` and make to
  create an instance for each connection.
  - `NoopKeepAliveHandler` now has two fielders to know whether
    a channel needs immediate disconnection or the connection has to be
    closed after receving all responses in process.
- Fixed `HttpChannelPool` to determine if a session is healthy using
  `isActive()`. This change allows inflights requests to use
  HTTP/2 sessions before GOAWAY is sent or received.
- Fixed to correctly set keep alive headers for HTTP/1.1.

Result:

- You can now send `Connection: close` to close a connection after
  receiving a response.
- Fixes line#4471
- Fixes line#4454
- Fixes line#4131
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only able to take a look at client side for now 😅 Left some minor comments & questions 🙏

@@ -293,7 +293,7 @@ private PooledChannel acquireNowExact(PoolKey key, SessionProtocol protocol) {

private static boolean isHealthy(PooledChannel pooledChannel) {
final Channel ch = pooledChannel.get();
return ch.isActive() && HttpSession.get(ch).canSendRequest();
return ch.isActive() && HttpSession.get(ch).isActive();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand this change 😅 For instance if HttpResponseDecoder#disconnectWhenFinished = true, then I thought we don't want to use the connection.

Also, if the check here is more lenient than the check done right before sending a request, I fear that users will intermittently see unnecessary ClosedSessionExceptions

if (id >= MAX_NUM_REQUESTS_SENT || !session.canSendRequest()) {

ikhoon and others added 2 commits November 15, 2022 21:39
…ionPoolListener.java

Co-authored-by: jrhee17 <guins_j@guins.org>
@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Patch coverage: 85.41% and project coverage change: +0.01 🎉

Comparison is base (c34c6b5) 74.08% compared to head (2631a12) 74.09%.

❗ Current head 2631a12 differs from pull request most recent head 205befc. Consider uploading reports for the commit 205befc to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4531      +/-   ##
============================================
+ Coverage     74.08%   74.09%   +0.01%     
- Complexity    18185    18201      +16     
============================================
  Files          1537     1537              
  Lines         67469    67497      +28     
  Branches       8537     8553      +16     
============================================
+ Hits          49987    50015      +28     
+ Misses        13419    13415       -4     
- Partials       4063     4067       +4     
Impacted Files Coverage Δ
...ava/com/linecorp/armeria/client/ClientOptions.java 83.52% <ø> (ø)
.../java/com/linecorp/armeria/client/HttpSession.java 37.50% <ø> (+18.75%) ⬆️
...ecorp/armeria/server/ServerHttp2ObjectEncoder.java 68.88% <ø> (+0.13%) ⬆️
...orp/armeria/client/AbstractHttpRequestHandler.java 80.15% <66.66%> (+0.30%) ⬆️
...rp/armeria/client/Http1ClientKeepAliveHandler.java 93.75% <66.66%> (-6.25%) ⬇️
...rp/armeria/server/AbstractHttpResponseHandler.java 92.63% <66.66%> (-1.76%) ⬇️
.../linecorp/armeria/client/Http1ResponseDecoder.java 58.85% <72.72%> (+0.96%) ⬆️
...m/linecorp/armeria/client/HttpResponseDecoder.java 84.74% <75.00%> (-0.64%) ⬇️
...om/linecorp/armeria/client/HttpSessionHandler.java 82.72% <81.25%> (+0.30%) ⬆️
.../armeria/internal/common/NoopKeepAliveHandler.java 76.92% <85.71%> (-4.90%) ⬇️
... and 36 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -170,7 +172,6 @@ public void onGoAwaySent(int lastStreamId, long errorCode, ByteBuf debugData) {
@Override
public void onGoAwayReceived(int lastStreamId, long errorCode, ByteBuf debugData) {
// Should not reuse a connection that received a GOAWAY frame.
HttpSession.get(channel()).deactivate();
disconnectWhenFinished();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed disconnectWhenFinished() to invoke HttpSession.get(channel()).deactivate() internally.

@@ -293,7 +293,7 @@ private PooledChannel acquireNowExact(PoolKey key, SessionProtocol protocol) {

private static boolean isHealthy(PooledChannel pooledChannel) {
final Channel ch = pooledChannel.get();
return ch.isActive() && HttpSession.get(ch).canSendRequest();
return ch.isActive() && HttpSession.get(ch).canAcquire();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canAcquire() usually becomes false early than canSendRequest(). So canAcquire() would be a stricter rule.

ctx.channel().close();
} else {
// Stop receiving new requests.
handledLastRequest = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Handle max connection age for HTTP/1. is moved here.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation 😄 Will start looking at server-side code also tomorrow

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 👍 Left some minor nits and questions 🙏

@@ -80,6 +87,10 @@ final boolean tryComplete(@Nullable Throwable cause) {
} else {
completionFuture.completeExceptionally(cause);
}

if (needsDisconnection) {
ctx.channel().close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question) I thought channel close would be handled from HttpServerHandler at the following lines:

if (protocol.isMultiplex()) {
// Initiates channel close, connection will be closed after all streams are closed.
ctx.channel().close();
} else {
// Stop receiving new requests.
handledLastRequest = true;
if (unfinishedRequests.isEmpty()) {
ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE);
}
}

May I ask why we close the channel here? (I ask because I'm concerned that a channel#close may be invoked although unfinishedRequests remains for http1 requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no HTTP/1 specification for graceful shutdown. So I assumed we have two kinds of connection close.

  • Graceful shutdown mode: If a connection needs to be closed by KeepAliveHandler such as a max connection age or ServiceRequestContext.initiateConnectionShutdown(), new requests will be ignored and the connection is closed after completing all unfinished requests.
  • Force shutdown mode: If a user explicitly sets Connection: close in the response headers, it is assumed that the connection should be closed after sending the response.

Copy link
Contributor

@jrhee17 jrhee17 Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, understood as follows for HTTP/1

  1. If client-side sends Connection: close headers or ClientRequestContext#initiateConnectionShutdown is called then graceful shutdown is done
  2. If server-side sends Connection: close headers from service layer, then immediate shutdown is done

From the perspective of code maintenance/complexity I prefer that the behavior is unified, but I guess if users want different behaviors we can support both ways of shutdown as long as this difference is well documented.

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good, thanks!
Left some of my opinions. 😉

@@ -157,6 +158,9 @@ public void onNext(HttpObject o) {
setDone(false);
}
merged = mergeResponseHeaders(headers, reqCtx.additionalResponseHeaders());
if (merged.contains(HttpHeaderNames.CONNECTION, HttpHeaderValues.CLOSE.toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's a good idea to automatically interpret the connection header and close the connection even in HTTP/2 communication because

  • connection header is not actually used in HTTP/2 so worried about misguiding users that the header works in HTTP/2.
  • using "connection: close" in server-side is not recommended because it will forcefully close the HTTP/1.1 connections
  • we have InitiateConnectionShutdown which gracefully shuts down the connection.

How about closing the connection only if it's HTTP/1.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connection header is not actually used in HTTP/2 so worried about misguiding users that the header works in HTTP/2.

I understand what you are concerned about. Using Connection header is improper behavior and looks to give users too much power. However, I still see it as a useful feature for both HTTP/1 and HTTP/2. How about adding a section in our documentation like How to close a connection? to explain the behavior?

using "connection: close" in server-side is not recommended because it will forcefully close the HTTP/1.1 connections

Although it closes the connection after sending Connection: close header, I don't think it violates HTTP/1 protocol. Many HTTP/1 server implementations send Connection: close header when necessary.

How about closing the connection only if it's HTTP/1.1?

However, Armeria server is able to handle both HTTP/1 and HTTP/2 and it depends on a client which protocol is used. If we only support it for HTTP/1, users who want to close a connection on a specific situation have to dispatch their logic depending on the negotiated protocol.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it violates HTTP/1 protocol.

I didn't mean that it was violating the protocol. I meant that we have a better way to close the connection gracefully after completing all the responses.

Armeria server is able to handle both HTTP/1 and HTTP/2 and it depends on a client which protocol is used.

ServiceRequestContext.initiateConnectionShutdown() is exactly for this. 😄
It's because it's really weird to close the HTTP/2 connections with the connection: close header, we provide a better alternative.
I also found a similar issue. jetty/jetty.project#2788
They decided not to use the connection header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that we have a better way to close the connection gracefully after completing all the responses.

I see. But, technically, there is no graceful shutdown for HTTP 1.1 on the server side. We can close the connection after completing all pending requests but new requests are silently ignored. Clients never know if the request is executed or not. They may get ClosedSessionException.

Copy link
Contributor Author

@ikhoon ikhoon Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also found a similar issue. jetty/jetty.project#2788

I am still leaning toward using connection: close to close connections. To use .initiateConnectionShutdown(), they should propagate a request context to a place where a decision for disconnection is made. Further more, it is a little bit inconvenient for clients.

There were similar requests to using connection: close for HTTP/2 on GitHub and Stack Overflow. Some projects maintainers refused to address the requests but others just deleted connection: close and sent a GOAWAY frame.
golang/net@97aa3a5

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they should propagate a request context to a place where a decision for disconnection is made. Further more, it is a little bit inconvenient for clients.

I believe we can get the ClientRequestContext almost where we want. Calling a method on the context is not inconvenient compared to setting the header I believe.

When we have two different specs, I believe that providing the abstract layer for the action is the way to go instead of using one of the specs and translating it automatically in order to make it work in another spec.
After we introduce HTTP/3, HTTP/4, and so on, we are going to still use "connection: close" which is the spec from the oldest one. If we make a decision to deprecate HTTP/1.1, we won't probably deprecate "connection: close" because it still might be used in our server for whom use HTTP/2.
That being said, I respect your opinion. HTTP/1.1 is used for a while and some people might think "connection: close" is the way to close the connection in HTTP/2. So it's up to you. If you think using a connection header is better, I won't object. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to stick to using connection: close. Consensus is necessary before proceeding further. Let's listen to other folks' opinions. Let's keep this PR open until we reach a conclusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that there is no ServiceRequestContext while working on the other PR.
Do you need to add a nullable ServiceRequestContext to allow calling initiateConnectionShutdown()?

public AggregatedHttpResponse renderStatus(ServiceConfig config,
@Nullable RequestHeaders headers,
HttpStatus status,
@Nullable String description,
@Nullable Throwable cause) {
if (status.isContentAlwaysEmpty()) {
return AggregatedHttpResponse.of(ResponseHeaders.of(status));

As ServiceRequestContext can't be provided for protocol violations, we may send connection: close by default.

Side note: We need to fix ServerHttp1ObjectEncoder.writeErrorResponse() to close a connection after sending connection: close.

Copy link
Contributor

@minwoox minwoox Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a chat with the team and we have decided to close the connection of HTTP/2 (technically send a GOAWAY frame) when connection: close is specified in terms of improving user convenience.

@ikhoon ikhoon modified the milestones: 1.21.0, 1.22.0 Dec 16, 2022
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor comments only. Great job, @ikhoon !

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Thanks a lot for your hard work! 🙇

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor questions, looks good! 👍

@@ -76,6 +76,8 @@ public abstract class AbstractKeepAliveHandler implements KeepAliveHandler {
private boolean isMaxConnectionAgeExceeded;

private boolean isInitialized;
private boolean closed;
private boolean disconnectWhenFinished;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help but wonder if HttpSessionHandler, HttpResponseDecoder, and KeepAliveHandler each need to have their own flags disconnectWhenFinished. (also for server-side)

Having said this, I guess this can be improved in the future if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Their usage is slightly different. We can consolidate the flags into KeepAliveHandler to handle keep-alive status in one place.

@@ -80,6 +87,10 @@ final boolean tryComplete(@Nullable Throwable cause) {
} else {
completionFuture.completeExceptionally(cause);
}

if (needsDisconnection) {
ctx.channel().close();
Copy link
Contributor

@jrhee17 jrhee17 Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, understood as follows for HTTP/1

  1. If client-side sends Connection: close headers or ClientRequestContext#initiateConnectionShutdown is called then graceful shutdown is done
  2. If server-side sends Connection: close headers from service layer, then immediate shutdown is done

From the perspective of code maintenance/complexity I prefer that the behavior is unified, but I guess if users want different behaviors we can support both ways of shutdown as long as this difference is well documented.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the last of my comments 😄 Sorry I took so long to review. Thanks @ikhoon 👍 🙇 🍀

@@ -409,8 +409,7 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
// HTTP/1 doesn't support draining that signals clients about connection shutdown but still
// accepts in flight requests. Simply destroy KeepAliveHandler which causes next response
// to have a "Connection: close" header and connection to be closed after the next response.
destroyKeepAliveHandler();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit; I feel like we can just remove this method altogether in this class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't get the intention. Could you elaborate on it?

Copy link
Contributor

@jrhee17 jrhee17 Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we don't need to call the method anymore since KeepAliveHandler will remove itself on channel disconnection.
Ignore this comment if I'm misunderstanding anything though 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Besides disconnection, KeepAliveHandler for HTTP/1 is destroyed while upgrading. The case is converted by encoder.close() that closes the underlying KeepAliaveHandler as well.

// FIXME(trustin): Use a different verboseResponses for a different virtual host.

The code related to keep-alive is quite messy. Let me send a cleanup PR after the LINE OSS sprint. 😄

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@trustin trustin added this pull request to the merge queue Mar 8, 2023
Merged via the queue into line:master with commit 20af195 Mar 8, 2023
ikhoon added a commit to ikhoon/armeria that referenced this pull request May 8, 2023
…sponse

Motivation:

`connection: close` header that should be preserved is removed  while
an Armeria server response is converted Netty HTTP/1 response. We
allowed users to send `connection: close` line#4531. The connection is
correctly closed after fully writing the response but `connection:
close` isn't sent correctly.

Modifications:

- Make `KeepAliverHandler` closed when `connection: close` header is
  specified in the response headers.

Result:

In HTTP/1.1, if `connection: close` is set in response headers,
the value is correctly sent to clients.
@ikhoon ikhoon deleted the handle-connection-close-header branch May 25, 2023 10:03
trustin pushed a commit that referenced this pull request Jun 9, 2023
…response (#4864)

Motivation:

`connection: close` header that should be preserved is removed while an
Armeria server response is converted to a Netty HTTP/1 response. We
allowed users to send `connection: close` #4531. The connection was
correctly closed after fully writing the response but `connection:
close` wasn't sent correctly.

Modifications:

- Make `KeepAliverHandler` closed when `connection: close` header is
specified in the response headers.

Result:

In HTTP/1.1, if `connection: close` is set in response headers, the
value is correctly sent to clients.
ikhoon added a commit to be-hase/armeria that referenced this pull request Jun 12, 2023
…response (line#4864)

Motivation:

`connection: close` header that should be preserved is removed while an
Armeria server response is converted to a Netty HTTP/1 response. We
allowed users to send `connection: close` line#4531. The connection was
correctly closed after fully writing the response but `connection:
close` wasn't sent correctly.

Modifications:

- Make `KeepAliverHandler` closed when `connection: close` header is
specified in the response headers.

Result:

In HTTP/1.1, if `connection: close` is set in response headers, the
value is correctly sent to clients.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants