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

Provide a way for WebClient to send a hint to close http connections #4131

Closed
jrhee17 opened this issue Mar 3, 2022 · 1 comment · Fixed by #4531
Closed

Provide a way for WebClient to send a hint to close http connections #4131

jrhee17 opened this issue Mar 3, 2022 · 1 comment · Fixed by #4531
Milestone

Comments

@jrhee17
Copy link
Contributor

jrhee17 commented Mar 3, 2022

Some users may want to disable keep-alive explicitly.

Currently, armeria relies on server-side to send a connection: close when determining whether to close a connection upon request/response completion.

However, armeria doesn't allow users to specify the connection header when sending requests

It may be worth introducing an option which allows the client to send a hint to close connections.

  • It may also be worth thinking of whether we want to support http2 as well (using goaway frames)
  • We may also want to check whether armeria server currently handles such connection close hints correctly as well.
@jrhee17
Copy link
Contributor Author

jrhee17 commented Mar 3, 2022

cc. @ghkim3221

ikhoon added a commit to ikhoon/armeria that referenced this issue 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.
  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
@trustin trustin closed this as completed in 20af195 Mar 8, 2023
@trustin trustin added this to the 1.23.0 milestone Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants