Page MenuHomePhabricator

MultiHttpClient has hard-coded deprecated cURL option used by CdnCacheUpdate
Open, MediumPublic


According to cURL API documentation, HTTP1 pipelining is deprecated.

MultiHttpClient uses it (in a hard-coded way):

// Configure when to multiplex multiple requests onto single TCP handles
$pipelining = $opts['usePipelining'] ?? $this->usePipelining;
curl_multi_setopt( $this->cmh, CURLMOPT_PIPELINING, $pipelining ? 3 : 0 );

…and CdnCacheUpdate sets this option (otherwise nothing else seems to do it):

$http = MediaWikiServices::getInstance()->getHttpRequestFactory()
        ->createMultiClient( [ 'maxConnsPerHost' => 8, 'usePipelining' => true ] );
$http->runMulti( $reqs );

This results in persistent error logging for cache update jobs:

[2020-10-06T16:29:12.580126+09:00] error.ERROR: [{exception_id}] {exception_url}   ErrorException from line 451 of w/includes/libs/http/MultiHttpClient.php: PHP Warning: curl_multi_setopt(): CURLPIPE_HTTP1 is no longer supported {"exception":"[object] (ErrorException(code: 0): PHP Warning: curl_multi_setopt(): CURLPIPE_HTTP1 is no longer supported at w/includes/libs/http/MultiHttpClient.php:451)","exception_id":"04ff6675aa6ed0a61e67af18","exception_url":"[no req]","caught_by":"mwe_handler"} []

Again, according to the documentation linked above the new default is to enable HTTP/2 multiplexing. Would it not make sense to therefore remove the curl_multi_setopt() call altogether?

Event Timeline

Reedy added a subscriber: Reedy.

The deprecation has been in since PHP 7.4, and is obviously in PHP 8.0 too

Using CURLPIPE_HTTP1 is deprecated, and is no longer supported as of cURL 7.62.0.

Looking at Debian.. It only affects >= buster 7.52 7.64

Change 644640 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Stop setting CURLMOPT_PIPELINING on curl >= 7.62.0

I don't think it's feasible to purge Squid with non-pipelined HTTP. The rationale for deprecating SquidPurgeClient and switching CdnCacheUpdate to cURL was that cURL was supposed to be able to do pipelining. We should probably revert those changes.

I guess maxConnsPerHost does offset the lack of pipelining somewhat. But with pipelining you can queue as many requests as will fit in the initial TCP window, typically thousands, whereas with maxConnsPerHost=8 you can only queue 8 requests before you have to wait for an RTT. It's not as clear cut as it seemed when I saw that change in April.

The rationale laid out in this blog post makes sense but I am not sure it is feasible to stay on cURL 7.61 and below forever or backport changes from newer versions.

The obvious alternative for ‘moving on’ is HTTP/2 multiplexing, but the problem here seems to be Squid which still has no HTTP/2 support nor any time-table for it. Varnish and Apache Traffic Server both support HTTP/2 for inbound connexions (Varnish does h2c only, as according to the developers it will never do TLS termination). cURL obviously supports HTTP/2 (but h2c support is a question mark).

Tentatively proposing as 1.36 blocker. I don't know whether there is an adequate fallback by default, but at the very least it shouldn't warn this way and it's going to be significantly more common for the audience of 1.36 than of 1.35 given more people upgrading PHP by then.

Based on @tstarling 's analysis it seems the fallback without pipelining would indeed likely be inadequate, even more so considering the use case for Squid seems fairly well correlated with wanting something more scalable.

This prevents edits using VE von MW Docker 1.35

When saving, the editor outputs an error as a warning is added to the returned json.

"Fix" is to disable php warnings, e.g. error_reporting(0); in LocalSettings

Change 720704 had a related patch set uploaded (by Paladox; author: Paladox):

[mediawiki/core@master] Remove deprecated CURLPIPE_HTTP1