jeckman / YouTube-Downloader

PHP script for downloading videos from youtube; also parsing youtube feed into RSS enclosures for podcatchers
GNU General Public License v2.0
895 stars 474 forks source link

Set curl timeout in config #351

Open jeckman opened 6 years ago

jeckman commented 6 years ago

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 commented 6 years ago

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

Potential targets:

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

muiton commented 6 years ago

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

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

Art4 commented 5 years ago

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

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

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 commented 5 years ago

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 commented 5 years ago

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

Art4 commented 5 years ago

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