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

Add TCP Fast Open support #1375

Merged
merged 7 commits into from
Apr 5, 2021
Merged

Add TCP Fast Open support #1375

merged 7 commits into from
Apr 5, 2021

Conversation

Scottmitch
Copy link
Member

Motivation:
Netty recently added support for
TCP fast open on the client, and
has had server support for a while. We currently don't expose the
necessary configuration knobs to enable this feature, but it may be able
to reduce round trips and reduce connection setup latency.

Modifications:

  • Add socket options to configure client and server TCP fast open.
  • Add mechanism to configure the listen/acceptor socket options, and use
    this for the backlog option which was an outlier.

Result:
TCP Fast Open can be enabled for client and server. Note that it is only
supported by Netty's native EPOLL transport on linux, and only
initiated on the client when TLS is enabled.

Scottmitch
Scottmitch commented Feb 17, 2021
View reviewed changes
@bondolo
Copy link
Contributor

bondolo commented Feb 18, 2021

Should include enhancements to this docs section: https://apple.github.io/servicetalk//servicetalk/SNAPSHOT/performance.html#socket-and-transport-options

idelpivnitskiy
idelpivnitskiy approved these changes Feb 19, 2021
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM after the build passes (awaiting netty release), thanks!

idelpivnitskiy
idelpivnitskiy reviewed Feb 19, 2021
View reviewed changes
servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/TcpFastOpenTest.java
}

@Test
public void requestSucceedsEvenIfTcpFastOpenNotEnabledOrSupported() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Btw, this test proves that it's safe to have this option always enabled. WDYT about making it on by default (opt-out)?

Copy link
Member Author

@Scottmitch Scottmitch Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, this test proves that it's safe to have this option always enabled.

Yes that is the goal. It shouldn't break anything even if not supported, and not enabled by the peer. If this test fails then we may want to consider more strict validation (e.g. throw error at build time if not supported).

WDYT about making it on by default (opt-out)?

Netty logs warnings if you enable an option that isn't supported, and also if it is supported users may not want to have it always enabled (e.g. idempotent, extra state tracking, etc.).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. What if we move it down to the transport, where we have access to Epoll.isAvailable() check (or Channel type) and know that the Channel is secure? For example, SslClientChannelInitializer might be a good place, if we pass the user-defined options there as well (to check that user did not explicitly set this option).

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting idea. I propose we proceed by adding the feature as opt-in first, and after we get some data we can consider enabling by default in a followup PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading around privacy concerns with TFO [1], makes me think that it's probably better left as opt-in.

  1. https://arxiv.org/pdf/1905.03518.pdf

tkountis
tkountis approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@tkountis tkountis left a comment

Choose a reason for hiding this comment

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

LGTM

bondolo
bondolo reviewed Feb 22, 2021
View reviewed changes
Copy link
Contributor

@bondolo bondolo left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't feel confident to approve.

@Scottmitch Scottmitch marked this pull request as ready for review March 10, 2021 06:44
@Scottmitch
Copy link
Member Author

depends upon netty/netty#11077

@Scottmitch
Add TCP Fast Open support
4d92ff1 
Motivation:
Netty recently added support for
[TCP fast open](https://tools.ietf.org/html/rfc7413) on the client, and
has had server support for a while. We currently don't expose the
necessary configuration knobs to enable this feature, but it may be able
to reduce round trips and reduce connection setup latency.

Modifications:
- Add socket options to configure client and server TCP fast open.
- Add mechanism to configure the listen/acceptor socket options, and use
  this for the backlog option which was an outlier.

Result:
TCP Fast Open can be enabled for client and server. Note that it is only
supported by Netty's native EPOLL transport on linux, and only
initiated on the client when TLS is enabled.
@Scottmitch
default implementation in builder base class
440d2e5
@Scottmitch
review comments, improve ChannelOpt<->SocketOpt conversion
0e4c107
@Scottmitch
review comments
a022609
@Scottmitch
use Theory
3a9d91e
@Scottmitch
checkstyle
bd67c62
@Scottmitch
fix incorrect FromDataPoints
115621f
@Scottmitch Scottmitch merged commit 808b31c into apple:main Apr 5, 2021
@Scottmitch Scottmitch deleted the tls_fastopen branch April 5, 2021 18:03
bondolo pushed a commit to bondolo/servicetalk that referenced this pull request Apr 6, 2021
@Scottmitch @bondolo
Add TCP Fast Open support (apple#1375)
1f38bf2 
Motivation:
Netty recently added support for
[TCP fast open](https://tools.ietf.org/html/rfc7413) on the client, and
has had server support for a while. We currently don't expose the
necessary configuration knobs to enable this feature, but it may be able
to reduce round trips and reduce connection setup latency.

Modifications:
- Add socket options to configure client and server TCP fast open.
- Add mechanism to configure the listen/acceptor socket options, and use
  this for the backlog option which was an outlier.

Result:
TCP Fast Open can be enabled for client and server. Note that it is only
supported by Netty's native EPOLL transport on linux, and only
initiated on the client when TLS is enabled.
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Jun 21, 2021
@idelpivnitskiy
Remove backlog(int) builder option that were deprecated in apple#1375
d5cd611 
Motivation:

`backlog(int)` builder option was deprecated in apple#1375 and can be removed
now. Users should use `listenSocketOption(SocketOption, Object)` as an
alternative.

Modifications:

- Remove `backlog(int)` from all server builders;

Result:

No deprecated `backlog(int)` server builder option.
@idelpivnitskiy idelpivnitskiy mentioned this pull request Jun 21, 2021
Merged
idelpivnitskiy added a commit that referenced this pull request Jun 22, 2021
@idelpivnitskiy
Remove backlog(int) server builder option that was deprecated in #1375
edd0fd2 
 (#1636)

Motivation:

`backlog(int)` builder option was deprecated in #1375 and can be removed
now. Users should use `listenSocketOption(SocketOption, Object)` as an
alternative.

Modifications:

- Remove `backlog(int)` from all server builders;

Result:

No deprecated `backlog(int)` server builder option.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bondolo bondolo bondolo left review comments

@idelpivnitskiy idelpivnitskiy idelpivnitskiy approved these changes

@tkountis tkountis tkountis approved these changes

Assignees

@Scottmitch Scottmitch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Scottmitch @bondolo @tkountis @idelpivnitskiy