lexiforest / curl_cffi

Python binding for curl-impersonate fork via cffi. A http client that can impersonate browser tls/ja3/http2 fingerprints.
https://curl-cffi.readthedocs.io/
MIT License
2.54k stars 269 forks source link

[BUG] stream: Setting `timeout` to float does not set connect timeout in 0.6.0b9 #237

Closed coletdjnz closed 9 months ago

coletdjnz commented 10 months ago

Describe the bug A clear and concise description of what the bug is.

When using stream mode and pass a float to the timeout parameter, it only sets the read timeout and not connect timeout (CURL_OPT_CONNECTTIMEOUT_MS).

Previously in 0.5.10 it set the connect timeout and not read timeout (read timeout was addressed in https://github.com/yifeikong/curl_cffi/commit/c5f840c538c623361c80f2dce40d3c620afb3b13).

Per requests timeout handling, which curl_cffi appears to be following: https://requests.readthedocs.io/en/latest/user/advanced/#timeouts

If you specify a single value for the timeout, like this: r = requests.get('https://github.com', timeout=5) The timeout value will be applied to both the connect and the read timeouts. Specify a tuple if you would like to set the values separately: r = requests.get('https://github.com', timeout=(3.05, 27))

https://github.com/yifeikong/curl_cffi/blob/bf563184065e13860ee7fa9e89c7e6fbfcbff619/curl_cffi/requests/session.py#L345-L363

0.5.10: https://github.com/yifeikong/curl_cffi/blob/bb7ceba1e1bb046c0b9f53e68b3e7892f7bfa017/curl_cffi/requests/session.py#L312-L327 See last line - it sets CONNECTTIMEOUT for stream

To Reproduce

def test_stream_connect_timeout():
    s = requests.Session()
    with pytest.raises(requests.RequestsError) as e:
        # hangs for curl default CONNECTTIMEOUT
        s.get("http://10.255.255.255", stream=True, timeout=1.0)

Expected behavior A clear and concise description of what you expected to happen.

Both read and connect timeout should be set to the float value passed to timeout in stream mode (with a tuple used to specify each individually).

Versions

Additional context

sync

perklet commented 10 months ago

Things were messed up a little bit, I will add it back.

perklet commented 9 months ago

Fixed in 0.6.0