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

Enable tcp fast open by default if available #1246

Open
wants to merge 6 commits into
base: dev/3.0.0
Choose a base branch
from

Conversation

AlexProgrammerDE
Copy link

@electronicboy
Copy link
Member

The AccessController is going bye bye, and so idk what we want to do on that front; but, given how useless of a feature TCP fast open seems to be, I'm not sure if there's much benefit to expanding this, we also have no mechanism of updating existing config files with new defaults, etc

@AlexProgrammerDE
Copy link
Author

Well AccessController is still used in two places in Velocity:

AccessController.doPrivileged(new PrivilegedAction<Void>() {
and I believe netty also uses it in a few places.
You could also get this option using natives like epoll does: https://github.com/netty/netty/blob/e8176601344b1c22e0f6c1dfc2861d7a92f54030/transport-native-epoll/src/main/c/netty_epoll_native.c#L606-L610
I don't think changing existing installations to enable fast-open is necessary. If you wanted to, you could create a new config option with it enabled by default and then adoption of this would be a lot higher.

@4drian3d 4drian3d added the type: feature New feature or request label Feb 17, 2024
@zlataovce
Copy link
Member

could this use the existing Epoll#isTcpFastOpenClientSideAvailable + Epoll#isTcpFastOpenServerSideAvailable API?

@AlexProgrammerDE
Copy link
Author

It could and I've taken a bit inspiration from that code, but I've opted not to do it because the method contract of those methods is that it also checks if Epoll is available. If Velocity uses different transports in the future that also support TFO (Such as IOUring, which also has IOUring#isTcpFastOpenClientSideAvailable + IOUring#isTcpFastOpenServerSideAvailable) then it would be problematic. I've done it like this to future-proof the detection code.

@zlataovce
Copy link
Member

does this check work for kqueue too? kqueue is also supported in Velocity and it's capable of doing fast open as well

I think it'd be better to make the checks in TransportType per transport with the proper Netty API methods

@AlexProgrammerDE
Copy link
Author

Alright done, it's now checking it on transport level.

@AlexProgrammerDE
Copy link
Author

One way this could be enabled by default is splitting the tcp-fast-open config key into tcp-fast-open-client and tcp-fast-open-server for finer control and that way also removing the tcp-fast-open key, which has always been false by default.

@electronicboy
Copy link
Member

We have 0 mechanism for modifying peoples existing config files

@AlexProgrammerDE
Copy link
Author

AlexProgrammerDE commented Feb 18, 2024

Yeah I guess that is not ideal. My idea above is a solution anyways. I believe TFO was disabled by default because enabling it without the transport supporting it, would send a warning message "Channel option unknown". Changing it to tcp-fast-open-client and tcp-fast-open-server would then enable it even if it was disabled by default before. I believe this is unlikely to break any setups. Maybe just write in the release notes that the config option was removed and there are two new ones that are true by default?

@electronicboy
Copy link
Member

We don't really have release notes, somebody would need to write a migration but making both sides configurable just seems bleh; I forgot we had the migration stuff, the config cruddery is a mess, but, if we want to enable by default, I'd much rather we just wrote a migration to flip it under the assertion that it shouldn't blow up for people, but, idk what to expect here as I can imagine somebody will find an environment that this blows up in.

I'm honestly just apprehensive around such a feature given most peoples servers aren't even setup to utilise it

@AlexProgrammerDE
Copy link
Author

I've now added a migration for flipping the value. I'm not sure if Paper supports TFO, but if it doesn't then I'll look into making a PR to Paper to enable TFO by default.

@xism4
Copy link
Contributor

xism4 commented May 4, 2024

We don't really have release notes, somebody would need to write a migration but making both sides configurable just seems bleh; I forgot we had the migration stuff, the config cruddery is a mess, but, if we want to enable by default, I'd much rather we just wrote a migration to flip it under the assertion that it shouldn't blow up for people, but, idk what to expect here a

The TFO is unnecessary if the backend does not have it, besides it is poorly implemented, since the levels as such do not exist. Rather, they are the connections per second that can be accepted by the mechanism itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants