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

Cannot get connection information when using TCP Fast Open #1332

Closed
irl opened this Issue Mar 16, 2017 · 17 comments

Comments

Projects
None yet
4 participants
@irl

irl commented Mar 16, 2017

I did this

Using the easy interface and the CURLOPT_TCP_FASTOPEN option, fetch a webpage. Then look for CURLINFO_LOCAL_PORT.

#include <stdio.h>
#include <curl/curl.h>

int main(void)
{
  CURL *curl;
  CURLcode res;

  curl = curl_easy_init();
  if(curl) {
    curl_easy_setopt(curl, CURLOPT_URL, "http://pathspider.net/");
    curl_easy_setopt(curl, CURLOPT_TCP_FASTOPEN, 1);
    res = curl_easy_perform(curl);

    if(CURLE_OK == res) {
      long *ct;
      res = curl_easy_getinfo(curl, CURLINFO_LOCAL_PORT, &ct);

      if((CURLE_OK == res) && ct == NULL) {
        printf("The operating was successful but we got nothing!\n");
      } else {
        printf("We used local port: %d\n", *ct);
      }
    }

    curl_easy_cleanup(curl);
  }
  return 0;
}

Instead of getting a port, I got a NULL, which is suboptimal for my use case.

I expected the following

A source port.

curl/libcurl version

curl 7.52.1 (x86_64-pc-linux-gnu) libcurl/7.52.1 OpenSSL/1.0.2k zlib/1.2.8 libidn2/0.16 libpsl/0.17.0 (+libidn2/0.16) libssh2/1.7.0 nghttp2/1.18.1 librtmp/2.3
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL

operating system

Debian testing (curl 7.52.1-3)

@jay

This comment has been minimized.

Member

jay commented Mar 16, 2017

That looks wrong, it doesn't write a pointer it writes a long to the address you pass it

--- a	Thu Mar 16 17:02:35 2017
+++ b	Thu Mar 16 17:08:38 2017
@@ -13,13 +13,10 @@
     res = curl_easy_perform(curl);
 
     if(CURLE_OK == res) {
-      long *ct;
+      long ct;
       res = curl_easy_getinfo(curl, CURLINFO_LOCAL_PORT, &ct);
-
-      if((CURLE_OK == res) && ct == NULL) {
-        printf("The operating was successful but we got nothing!\n");
-      } else {
-        printf("We used local port: %d\n", *ct);
+      if(CURLE_OK == res) {
+        printf("We used local port: %d\n", ct);
       }
     }
 
@bagder

This comment has been minimized.

Member

bagder commented Mar 16, 2017

(I took this as an opportunity and added this as an example to the man page in 49f7b13)

@jay

This comment has been minimized.

Member

jay commented Mar 16, 2017

Despite the example he still has a point, it looks like the local port isn't saved when fastopen is used, see https://github.com/curl/curl/blob/curl-7_53_1/lib/connect.c#L674

@bagder

This comment has been minimized.

Member

bagder commented Mar 16, 2017

hm, I wonder why that was made. I can't remember any discussions around that. Surely that would all work fine even with TFO ?

@ghedo

This comment has been minimized.

Member

ghedo commented Mar 16, 2017

So, I don't quite remember the reasoning behind this, but one possible explanation is that when Curl_updateconninfo() is called the connection may not actually be established yet. With TFO on Linux this would only happen after the first sendto().

@ghedo

This comment has been minimized.

Member

ghedo commented Mar 16, 2017

That said, there might be a better way of avoiding that problem.

@jay

This comment has been minimized.

Member

jay commented Mar 16, 2017

AFAICT it's only called after the connection is made... or at least what I interpret that to be, I'm not familiar with TFO. It's only called these two places:
https://github.com/curl/curl/blob/curl-7_53_1/lib/connect.c#L808
https://github.com/curl/curl/blob/curl-7_53_1/lib/url.c#L6843

@bagder

This comment has been minimized.

Member

bagder commented Mar 16, 2017

And even if it was called before the connection is made, all the function calls within Curl_updateconninfo check return codes and take precautions so they should fail gracefully...

@ghedo

This comment has been minimized.

Member

ghedo commented Mar 16, 2017

@jay define "connection is made" :) The TCP connection is initiated on the wire when curl starts sending the HTTP request (or whatever). That is when sendto() is called. Before then the socket is not connected.

@bagder doesn't look graceful to me, given the error = SOCKERRNO, curl will not proceed https://github.com/curl/curl/blob/curl-7_53_1/lib/connect.c#L823 (edit: nvm, missed a return there).

@ghedo

This comment has been minimized.

Member

ghedo commented Mar 16, 2017

It does print a bunch of failf() though, which is not exactly nice during normal (for TFOl operation...

@ghedo

This comment has been minimized.

Member

ghedo commented Mar 16, 2017

And even if they do fail nicely, that still leaves the problem of getinfo returning nothing. Should Curl_updateconninfo() be called again after the initial send for TFO and that condition removed?

@irl

This comment has been minimized.

irl commented Mar 16, 2017

Sorry for the example, I was attempting to replicate the problem I was finding in pycurl in C.

I attempted the following in sendf.c but this didn't work:

...
#ifdef MSG_FASTOPEN /* Linux */
  if(conn->bits.tcp_fastopen) {
    bytes_written = sendto(sockfd, mem, len, MSG_FASTOPEN,
                           conn->ip_addr->ai_addr, conn->ip_addr->ai_addrlen);
    conn->bits.tcp_fastopen = FALSE;
    Curl_persistconninfo(conn);
  }
  else
#endif
...

I guess this is probably not how these functions work.

Edit: maybe this should have been Curl_updateconninfo() instead of Curl_persistconninfo()?
Edit: I guess it's not that easy, something about transport not being connected.

@ghedo

This comment has been minimized.

Member

ghedo commented Mar 17, 2017

@irl hmm, that could be caused by the fact that a non-blocking socket is used. Another experiment would be to call Curl_updateconninfo() after the first successful send. Basically just before https://github.com/curl/curl/blob/master/lib/sendf.c#L373 but only if bytes_written is greater than 0.

I can't test right now, I'll look into this later.

@irl

This comment has been minimized.

irl commented Mar 17, 2017

Yep, non-blocking socket issue. Adding Curl_updateconninfo() after the first successful send works in the case that TCP fast open failed and a non-TFO TCP connection was used, but doesn't work in the case that TFO was successful and only one send is ever performed (because the whole GET request fits on the SYN).

I'm playing with setting a needs_update bit and adding Curl_updateconninfo() after both the first successful write and the first successful read.

@irl

This comment has been minimized.

irl commented Mar 17, 2017

Pull request #1335 should solve this, just doing a little more testing.

Edit: nope, needs more work. It's the right idea, and if you drop the clearing of the flags it works, but I think the test conditions in the if statements are wrong.

irl added a commit to irl/curl that referenced this issue Aug 30, 2017

sendf.c: Makes further attempts at updateconninfo if first failed
When using non-blocking sockets, such as TCP Fast Open sockets,
connection information may not be available at the time the
socket is "created". For TCP Fast Open, the creation can even be
a NOP as far as the network is concerned.

This change adds tests to the Curl_send and Curl_recv functions
to make further attempts at updating the connection information
for as long as local_port in the connectdata struct is zero.

Once a call to Curl_updateconninfo has succeeded, and so updated
the local_port field, there will be no further calls to
Curl_updateconninfo.

Closes: curl#1332

irl added a commit to irl/curl that referenced this issue Aug 30, 2017

sendf.c: Makes further attempts at updateconninfo if first failed
When using non-blocking sockets, such as TCP Fast Open sockets,
connection information may not be available at the time the
socket is "created". For TCP Fast Open, the creation can even be
a NOP as far as the network is concerned.

This change adds tests to the Curl_send and Curl_recv functions
to make further attempts at updating the connection information
for as long as local_port in the connectdata struct is zero.

Once a call to Curl_updateconninfo has succeeded, and so updated
the local_port field, there will be no further calls to
Curl_updateconninfo.

Closes: curl#1332

irl added a commit to irl/curl that referenced this issue Sep 15, 2017

sendf.c: Makes further attempts at updateconninfo if first failed
When using non-blocking sockets, such as TCP Fast Open sockets,
connection information may not be available at the time the
socket is "created". For TCP Fast Open, the creation can even be
a NOP as far as the network is concerned.

This change adds tests to the Curl_send and Curl_recv functions
to make further attempts at updating the connection information
for as long as local_port in the connectdata struct is zero.

Once a call to Curl_updateconninfo has succeeded, and so updated
the local_port field, there will be no further calls to
Curl_updateconninfo.

Closes: curl#1332

irl added a commit to irl/curl that referenced this issue Sep 15, 2017

sendf.c: Makes further attempts at updateconninfo if first failed
When using non-blocking sockets, such as TCP Fast Open sockets,
connection information may not be available at the time the
socket is "created". For TCP Fast Open, the creation can even be
a NOP as far as the network is concerned.

This change adds tests to the Curl_send and Curl_recv functions
to make further attempts at updating the connection information
for as long as local_port in the connectdata struct is zero.

Once a call to Curl_updateconninfo has succeeded, and so updated
the local_port field, there will be no further calls to
Curl_updateconninfo.

Closes: curl#1332

@stale stale bot added the stale label Sep 16, 2017

@stale

This comment has been minimized.

stale bot commented Sep 16, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@irl

This comment has been minimized.

irl commented Sep 17, 2017

This is still an issue, a PR is waiting for review in #1847.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 29, 2018

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