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

Jump to bottom

Add support for client-side TCP FastOpen to KQueue MacOS #11560

Merged

chrisvest merged 12 commits into netty:4.1 from chrisvest:41-kqueue-tfo

Aug 12, 2021

Contributor

chrisvest commented Aug 6, 2021 •

edited

Motivation:
The MacOS-specific connectx(2) system call make it possible to establish client-side connections with TCP FastOpen.

Modification:
Add support for TCP FastOpen to the KQueue transport, and add the connectx(2) system call to BsdSocket.

Result:
It's now possible to use TCP FastOpen when initiating connections on MacOS.

chrisvest requested review from normanmaurer and NiteshKant

August 6, 2021 14:15

Scottmitch reviewed

View reviewed changes

transport-native-kqueue/src/main/java/io/netty/channel/kqueue/BsdSocket.java Outdated Show resolved Hide resolved

transport-native-kqueue/src/main/java/io/netty/channel/kqueue/BsdSocket.java Outdated Show resolved Hide resolved

transport-native-kqueue/src/main/java/io/netty/channel/kqueue/BsdSocket.java Outdated Show resolved Hide resolved

transport-native-kqueue/src/main/c/netty_kqueue_bsdsocket.c Outdated Show resolved Hide resolved

transport-native-kqueue/src/main/c/netty_kqueue_bsdsocket.c Show resolved Hide resolved

transport-native-kqueue/src/main/c/netty_kqueue_bsdsocket.c

    
                  const struct iovec* iov = (const struct iovec*) iovAddress;

                  unsigned int iovcnt = (unsigned int) iovCount;

                  size_t len = (size_t) iovDataLength;

                  int result = connectx(socket, &endpoints, SAE_ASSOCID_ANY, flags, iov, iovcnt, &len, NULL);

Member

Scottmitch Aug 6, 2021

nit: if result isn't needed outside the conditional consider inlining the call to connectx

Contributor Author

chrisvest Aug 9, 2021

It takes a lot of arguments, so I find this style easier to read.

transport-native-kqueue/src/main/c/netty_kqueue_bsdsocket.c

    
                      endpoints.sae_dstaddrlen = dstaddrlen;

                  }

                  int socket = (int) socketFd;

Member

Scottmitch Aug 6, 2021

nit: these calls can be inlined, may make it easier for compiler to remove intermediate storage and less intermediate variable noise in the code if they are only used once.

Contributor Author

chrisvest Aug 9, 2021

I don't understand this concern with local variables.

Member

Scottmitch Aug 10, 2021

fine to leave as is but the goal would be to decrease noise in the code and aid readability (why are there multiple variables, can their values change independently, what is the memory lifetime/location of each, etc.).

transport-native-kqueue/src/main/c/netty_kqueue_bsdsocket.c Outdated Show resolved Hide resolved

Contributor Author

chrisvest commented Aug 9, 2021

@Scottmitch addressed your comments, please take a look.

Scottmitch reviewed

View reviewed changes

transport-native-kqueue/src/main/java/io/netty/channel/kqueue/Native.java Outdated Show resolved Hide resolved

transport-native-kqueue/src/main/java/io/netty/channel/kqueue/BsdSocket.java Outdated Show resolved Hide resolved

transport-native-kqueue/src/main/c/netty_kqueue_bsdsocket.c Show resolved Hide resolved

transport-native-kqueue/src/main/c/netty_kqueue_bsdsocket.c

    
                      endpoints.sae_dstaddrlen = dstaddrlen;

                  }

                  int socket = (int) socketFd;

Member

Scottmitch Aug 10, 2021

fine to leave as is but the goal would be to decrease noise in the code and aid readability (why are there multiple variables, can their values change independently, what is the memory lifetime/location of each, etc.).

chrisvest added this to the 4.1.67.Final milestone

Contributor Author

chrisvest commented Aug 10, 2021

@Scottmitch @NiteshKant @normanmaurer I've verified that this works locally; was able to establish a TFO connection from a MacOS host, running with kqueue, to a Linux guest VM running with Epoll, and see the SslHandlers ClientHello get transferred with the SYN packet on repeated connections.

chrisvest marked this pull request as ready for review

August 10, 2021 13:20

Scottmitch approved these changes

View reviewed changes

Member

Scottmitch left a comment •

edited

lgtm assuming this has been tested/verified locally ... ideally a unit test would be added to verify that nothing breaks when these options are used (even if we don't verify that app data is included in the SYN)

Contributor Author

chrisvest commented Aug 11, 2021

@Scottmitch yeah, been working on adding this to the kqueue test permutations

chrisvest force-pushed the 41-kqueue-tfo branch from 932a5d9 to c735f6c Compare

August 11, 2021 10:03

chrisvest added 11 commits

August 11, 2021 13:41


          Add connectx(2) to BsdSocket

6e3b3fb


          Draft of adding client-side support for TCP FastOpen to the MacOS KQu…

dda2e03

…eue transport


          Make one of the BsdSocket.connectx private

3a092ea


          Cache the CONNECT_RESUME_ON_READ_WRITE and CONNECT_DATA_IDEMPOTENT va…

1f111b4

…lues as class constants in Native


          Explain the "unspecified source interface" value used with connectx

1faac56


          Improve BsdSocket.connectx null handling

8c4d471


          Throw exceptions from BsdSocket.connectx if address conversion fails

a84f5fc


          Fix java compatibility error in BsdSocket

f253779


          Fix a number of issues with the KQueue TFO support

aeec6fa


          When using connectx for TFO on non-blocking sockets, it will return E…

14306fc

…INPROGRESS

Make sure we handle that as a successful call.


          Add TCP FastOpen to the KQueue test permutations

Also fix a bug where using TFO with KQueue would prematurely consider the socket connected.
Instead, the socket should assume to be connect-in-progress when using TFO since all our sockets are non-blocking.

chrisvest force-pushed the 41-kqueue-tfo branch from c735f6c to 4577097 Compare

August 11, 2021 11:41

NiteshKant reviewed

View reviewed changes

Member

NiteshKant left a comment

As C is greek for me, I am relying on Scott's review of the C code. Overall looks good to me, just a few questions about the return value.

transport-native-kqueue/src/main/java/io/netty/channel/kqueue/BsdSocket.java Outdated Show resolved Hide resolved

transport-native-kqueue/src/main/java/io/netty/channel/kqueue/KQueueSocketChannel.java Outdated Show resolved Hide resolved


          Accurately determine if a connection is in-progress or not, when usin…

075acbb

…g connectx/TCP FastOpen on MacOS

chrisvest merged commit f750e2e into netty:4.1

chrisvest deleted the 41-kqueue-tfo branch

August 12, 2021 11:38

chrisvest added a commit that referenced this pull request


          Add support for client-side TCP FastOpen to KQueue MacOS (#11560)

25699e4

Motivation:
The MacOS-specific `connectx(2)` system call make it possible to establish client-side connections with TCP FastOpen.

Modification:
Add support for TCP FastOpen to the KQueue transport, and add the `connectx(2)` system call to `BsdSocket`.

Result:
It's now possible to use TCP FastOpen when initiating connections on MacOS.

laosijikaichele pushed a commit to laosijikaichele/netty that referenced this pull request


          Add support for client-side TCP FastOpen to KQueue MacOS (netty#11560)

96a4c50

Motivation:
The MacOS-specific `connectx(2)` system call make it possible to establish client-side connections with TCP FastOpen.

Modification:
Add support for TCP FastOpen to the KQueue transport, and add the `connectx(2)` system call to `BsdSocket`.

Result:
It's now possible to use TCP FastOpen when initiating connections on MacOS.

laosijikaichele pushed a commit to laosijikaichele/netty that referenced this pull request


          Add support for client-side TCP FastOpen to KQueue MacOS (netty#11560)

fff2884

Motivation:
The MacOS-specific `connectx(2)` system call make it possible to establish client-side connections with TCP FastOpen.

Modification:
Add support for TCP FastOpen to the KQueue transport, and add the `connectx(2)` system call to `BsdSocket`.

Result:
It's now possible to use TCP FastOpen when initiating connections on MacOS.

raidyue pushed a commit to raidyue/netty that referenced this pull request


          Add support for client-side TCP FastOpen to KQueue MacOS (netty#11560)

5bcd780

Motivation:
The MacOS-specific `connectx(2)` system call make it possible to establish client-side connections with TCP FastOpen.

Modification:
Add support for TCP FastOpen to the KQueue transport, and add the `connectx(2)` system call to `BsdSocket`.

Result:
It's now possible to use TCP FastOpen when initiating connections on MacOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment