Skip to content
This repository has been archived by the owner on Jul 12, 2020. It is now read-only.

Set curl timeout in config #351

Open
jeckman opened this issue Nov 17, 2018 · 6 comments
Open

Set curl timeout in config #351

jeckman opened this issue Nov 17, 2018 · 6 comments

Comments

@jeckman
Copy link
Owner

jeckman commented Nov 17, 2018

Today there are a number of places where the curl option for timeout ( CURLOPT_TIMEOUT, CURLOPT_CONNECTTIMEOUT ) gets set to various values.

Ideally, on could set that in the config file and have it applied.

Impacted:
/src/Http/CurlClient.php line 85, which currently sets CURLOPT_CONNECTTIMEOUT to 3.
/src/Provider/Youtube/SignatureDecipher.php line 323 which sets CURLOPT_TIMEOUT to 10.
/src/Application/ControllerAbstract.php line 92, which sets CURLOPT_TIMEOUT to 3.
/src/Application/DownloadController.php line 199, which sets CURLOPT_TIMEOUT to 50.

Granted some of these are for different purposes, but it feels like we need a more clear single place to set all the CURLOPT stuff

@jeckman
Copy link
Owner Author

jeckman commented Nov 17, 2018

Not sure which all options we'd want to set in the config rather than in more specific locations.

Potential targets:

  • CURLOPT_TIMEOUT
  • CURLOPT_CONNECTTIMEOUT
  • CURLOPT_SSR_VERIFYPEER
  • CURLOPT_FOLLOWLOCATION

Less clear to me whether CURLOPT_NOBODY makes sense, or CURLOPT_INTERFACE which I think is only used once

@muiton
Copy link
Contributor

muiton commented Nov 26, 2018

I preffer using 30 seconds for timeout on my localhost as I have very lagging net connection. So, timeout option should be added.

As for

  • CURLOPT_SSR_VERIFYPEER
  • CURLOPT_FOLLOWLOCATION

I don't think anyone would need these to be reconfigured.

@Art4
Copy link
Collaborator

Art4 commented Dec 7, 2018

I'm improving our HTTP client atm and will tackle this issue. I recommend to let this options define via config:

  • CURLOPT_CONNECTTIMEOUT: 3
  • CURLOPT_SSL_VERIFYPEER: true
  • CURLOPT_SSL_VERIFYHOST: true
  • CURLOPT_FOLLOWLOCATION: true
  • CURLOPT_ENCODING
  • CURLOPT_TIMEOUT

CURLOPT_TIMEOUT is a bit special and could be split into multiple parts:

  • timeout_getsize: 3
  • timeout_mp3download: 50
  • timeout_websitedownload: 10
  • timeout_playerdownload: 10

And we have also a few calls of file_put_contents() and readfile() which relied on INI setting default_socket_timeout with a default of 60 seconds.

@Art4
Copy link
Collaborator

Art4 commented Dec 7, 2018

Based on the Guzzle request options I would recommend the following names in config that will be set to the HTTP client:

config leads to curl option
timeout: 60 CURLOPT_TIMEOUT: 60
verify: true CURLOPT_SSL_VERIFYPEER: true, CURLOPT_SSL_VERIFYHOST: true
curl[CURLOPT_INTERFACE]: IP CURLOPT_INTERFACE: IP
allow_redirects: true CURLOPT_FOLLOWLOCATION: true
connect_timeout: 3 CURLOPT_CONNECTTIMEOUT: 3
decode_content: "" CURLOPT_ENCODING: ""

@jeckman
Copy link
Owner Author

jeckman commented Dec 27, 2018

@Art4 Is this something you are working on as you rework some of the underlying use of curl, or is it already done?

@Art4
Copy link
Collaborator

Art4 commented Dec 27, 2018

@jeckman I'm not actively working on this atm, but the plan is to rework all curl functions into the CurlClient.

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

No branches or pull requests

3 participants