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

Fix appconnect time problematic accumulation when using TLS-in-TLS proxy #7274

Closed
wants to merge 3 commits into from

Conversation

@MrDwZ
Copy link

@MrDwZ MrDwZ commented Jun 17, 2021

Follow up this email list thread.
We use a TLS-in-TLS HTTPS proxy with proxy client auth required setup, the appconnect time accumulated twice and exceeds the total time.
I found this seems to caused by a potential bug in Curl_pgrsTime, the problem is when we use TLS-in-TLS proxy, Curl_ssl_connect_nonblocking called twice, as we need to build TLS connection to proxy and followed by destination. See this stack trace. The current implementation would be:

  1. When connect to proxy: t_appconnect = now - t_startsingle
  2. When connect to destination: t_appconnect = t_appconnect + (now - t_startsingle)

My commit changes t_appconnect to only record the time that TLS has been established to destination, as both TLS connection finished.

I know we accumulate some timerid because of redirection, any suggestion would be welcome if there's anything is wrong with this PR. :-)

I tested by building locally and the appconnect time is less than all the following time.
Before

curl $([my proxy config]) -v -s -w "@curl-format.txt"  -o /dev/null https://www.google.com 
  time_namelookup: 0.000108
       time_connect: 0.199598
    time_appconnect: 0.443684
   time_pretransfer: 0.233842
time_starttransfer: 0.273798
                    ----------
         time_total: 0.276055

After

curl $([my proxy config]) -v -s -w "@curl-format.txt"  -o /dev/null https://www.google.com
    time_namelookup:  0.000060
       time_connect:  0.199511
    time_appconnect:  0.227428
   time_pretransfer:  0.227497
 time_starttransfer:  0.329309
         time_total:  0.329540
@MrDwZ MrDwZ changed the title Fix appconnect time duplicate calculation when using TLS-in-TLS proxy Fix appconnect time duplicate accumulation when using TLS-in-TLS proxy Jun 17, 2021
@MrDwZ MrDwZ changed the title Fix appconnect time duplicate accumulation when using TLS-in-TLS proxy Fix appconnect time problematic accumulation when using TLS-in-TLS proxy Jun 17, 2021
@MrDwZ MrDwZ force-pushed the MrDwZ:master branch 4 times, most recently from bfd3cb4 to 97ff98f Jun 18, 2021
@MrDwZ MrDwZ force-pushed the MrDwZ:master branch from 97ff98f to 4702a46 Jun 18, 2021
bagder added a commit that referenced this pull request Jun 18, 2021
Introducing a 'isproxy' argument to the connect function so that it
knows wether to store the time stamp or not.

Reported-by: Yongkang Huang
Fixes #7274
@bagder
Copy link
Member

@bagder bagder commented Jun 18, 2021

Thanks! I'm not super happy about adding more special-purpose state bits to the Curl_easy struct, but after reading your PR and patch I created an alternative take on this (that should fix the same issue) that I'd love your comments on: #7275

@MrDwZ MrDwZ closed this Jun 18, 2021
@MrDwZ
Copy link
Author

@MrDwZ MrDwZ commented Jun 18, 2021

merge the discussion into #7275

bagder added a commit that referenced this pull request Jun 19, 2021
Introducing a 'isproxy' argument to the connect function so that it
knows wether to store the time stamp or not.

Reported-by: Yongkang Huang
Fixes #7274
Closes #7274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants