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.

Sign up for GitHub

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

Jump to bottom

Send Connection: close header when it is received. #4471

Closed
ikhoon opened this issue Oct 7, 2022 · 3 comments · Fixed by #4531
Closed

Send Connection: close header when it is received. #4471

ikhoon opened this issue Oct 7, 2022 · 3 comments · Fixed by #4531
Labels
defect
Milestone
1.23.0

Comments

@ikhoon
Copy link
Contributor

ikhoon commented Oct 7, 2022

If Connection: close is not set in HTTP/1.1 headers, we can assume the connection is persistent.
If a client sends Connection: close, HttpServerHandler refuses accepting additional requests from the connection.
But it does not send back Connection: close to the client. It would be fine in most cases because the client wanted to close the connection and it may close the connection regardless of the existence of Connection: close.
That being said, sending Connection: close clearly complies with the HTTP/1.1 protocol.
@ikhoon ikhoon added the defect label Oct 7, 2022
@ikhoon ikhoon added this to the 1.21.0 milestone Oct 12, 2022
ikhoon added a commit to ikhoon/armeria that referenced this issue Nov 11, 2022
@ikhoon
Allow sending Connection: close header
52f6d8c 
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
@ikhoon ikhoon mentioned this issue Nov 11, 2022
Merged
@jrhee17
Copy link
Contributor

jrhee17 commented Nov 15, 2022 •
edited
Loading

HTTP/1.1 defines the "close" connection option for the sender to
signal that the connection will be closed after completion of the
response. For example,

   Connection: close

in either the request or the response header fields indicates that
the connection SHOULD NOT be considered `persistent' (section 8.1)
after the current request/response is complete.

https://www.rfc-editor.org/rfc/rfc2616#section-14.10

So that other people reviewing the PR doesn't have to dig into the rfc.

@jrhee17
Copy link
Contributor

jrhee17 commented Nov 15, 2022

Having said this, I'm curious why we want to do this at the cost of sending an extra number of bytes.

Were there any edge-cases that were failing due to not sending Connection: close from server-side?

@ikhoon
Copy link
Contributor Author

ikhoon commented Nov 15, 2022

Were there any edge-cases that were failing due to not sending Connection: close from server-side?

No, it isn't. I realized Armeria doesn't send a Connection: close header while implementing Armeria backend for Play Framework. Anyway, sending "Connection: close` seems more common and correct behavior in HTTP/1.1 servers.

# Did not return "Connection: close"
curl --http1.1 https://www.google.com -I
curl --http1.1 https://facebook.com -I
curl --http1.1 https://github.com -I
# All servers sent back "Connection: close" header to the client
curl --http1.1 https://www.google.com -I -H "connection: close"
curl --http1.1 https://facebook.com -I -H "connection: close"
curl --http1.1 https://github.com -I -H "connection: close"

@ikhoon ikhoon modified the milestones: 1.21.0, 1.22.0 Dec 16, 2022
@ikhoon ikhoon modified the milestones: 1.22.0, 1.23.0 Feb 5, 2023
trustin pushed a commit that referenced this issue Mar 8, 2023
@ikhoon @jrhee17
Allow sending Connection: close header (#4531)
20af195 
* Allow sending `Connection: close` header

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 #4471
- Fixes #4454
- Fixes #4131

* Update core/src/test/java/com/linecorp/armeria/client/CountingConnectionPoolListener.java

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

* Rename isActive() into canAcquire() and invoked deactivate() whenever disconnectWhenFinish() is called

* Fix the flaky test

* Address comments by @jrhee17

* Disallow acquring when a connection is about to disconnect

* Allow only "connection: close" header

* Add comments for connection shutdown mode

* Address comments by @trustin

* Add assert

* Address comments by @jrhee17

* Fix recusive calls

* Remove cruft

---------

Co-authored-by: jrhee17 <guins_j@guins.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
1.23.0
Development

Successfully merging a pull request may close this issue.

Allow sending Connection: close header ikhoon/armeria
2 participants
@ikhoon @jrhee17