connect: add support for new TCP Fast Open API on Linux #2056
Conversation
lib/connect.c
Outdated
| @@ -1078,7 +1080,17 @@ static CURLcode singleipconnect(struct connectdata *conn, | |||
| rc = connect(sockfd, &addr.sa_addr, addr.addrlen); | |||
| } | |||
| #endif /* HAVE_BUILTIN_AVAILABLE */ | |||
| #elif defined(MSG_FASTOPEN) /* Linux */ | |||
| #elif defined(TCP_FASTOPEN_CONNECT) /* Linux >= 4.11 */ | |||
| int optval = 1; | |||
lib/connect.c
Outdated
| int optval = 1; | ||
|
|
||
| if(setsockopt(sockfd, IPPROTO_TCP, TCP_FASTOPEN_CONNECT, | ||
| (void *)&optval, sizeof(optval)) < 0) |
There was a problem hiding this comment.
column alignment should be used on continuation lines when possible
if(setsockopt(sockfd, IPPROTO_TCP, TCP_FASTOPEN_CONNECT,
(void *)&optval, sizeof(optval)) < 0)| @@ -360,7 +364,7 @@ ssize_t Curl_send_plain(struct connectdata *conn, int num, | |||
| available. */ | |||
| pre_receive_plain(conn, num); | |||
|
|
|||
| #ifdef MSG_FASTOPEN /* Linux */ | |||
| #if defined(MSG_FASTOPEN) && !defined(TCP_FASTOPEN_CONNECT) /* Linux */ | |||
| if(conn->bits.tcp_fastopen) { | |||
| bytes_written = sendto(sockfd, mem, len, MSG_FASTOPEN, | |||
| conn->ip_addr->ai_addr, conn->ip_addr->ai_addrlen); | |||
There was a problem hiding this comment.
although not specifically related to your changes i'd like to address how depending on which fastopen is used tcp_fastopen may be set false. For example conn->bits.tcp_fastopen = FALSE; in the line below this line (I can't select it in the reviewer because it's too out of context). Yet Mac TFO and now this new TFO it says true. What concerns me is other code seem to expect it one way or the other such as
https://github.com/curl/curl/blob/curl-7_56_1/lib/connect.c#L675
https://github.com/curl/curl/blob/curl-7_56_1/lib/connect.c#L780
was the intention for tcp_fastopen to always be set false or only set false in certain circumstances?
There was a problem hiding this comment.
In this case tcp_fastopen is set to false because the sendto() logic only needs to be run once per connection. In the other cases (macOS and TCP_FASTOPEN_CONNECT) there is no sendto() logic.
We could set it to false for the other cases immediately after connect(), but I don't think it's necessary.
|
i notice this is failing on test230 "HTTP GET multiply compressed content". I don't have that test, where did that come from? |
|
@jay : test230 is from commit dbcced8#diff-c3eb2d696b714d609a3f191d0b60bf94. It checks multiple content encoding. I already had the same problem once while developing this feature, but resubmitting the job was al right. I have tried to make it fail locally (outside travis) without success. I've just found a problem with data length (that can be random) and fixed it. Next travis runs will show if the error occurs again. |
|
ping @ghedo, would you mind rebasing this and fix the conflicts? |
The new API added in Linux 4.11 only requires setting a socket option before connecting, without the whole sento() machinery. Notably, this makes it possible to use TFO with SSL connections on Linux as well, without the need to mess around with OpenSSL (or whatever other SSL library) internals.
|
Done. |
|
Btw, you can use https://www.ghedini.me to test this, e.g. using tcpdump on the first curl run I see: and then running curl again (once we have received and cached the TFO cookie from the previous connection): |
|
Thanks! |
The new API added in Linux 4.11 only requires setting a socket option
before connecting, without the whole sento() machinery.
Notably, this makes it possible to use TFO with SSL connections on Linux
as well, without the need to mess around with OpenSSL (or whatever other
SSL library) internals.