Page MenuHomePhabricator
Create Task
Maniphest T264735

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

Description

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?

Details

ProjectBranchLines +/-Subject
mediawiki/coremaster+1 -1Remove deprecated CURLPIPE_HTTP1
mediawiki/coremaster+12 -2Stop setting CURLMOPT_PIPELINING on curl >= 7.62.0
Customize query in gerrit

Related Objects

Event Timeline

IijimaYun created this task.Oct 6 2020, 11:12 AM
Restricted Application added a subscriber: • Aklapper. · View Herald TranscriptOct 6 2020, 11:12 AM
IijimaYun added a subscriber: aaron.Oct 6 2020, 11:18 AM
Reedy added a project: Platform Team Workboards.Oct 6 2020, 1:47 PM
Reedy added projects: PHP 7.4 support, PHP 8.0 support.Oct 6 2020, 2:22 PM
Reedy added a subscriber: Reedy.

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

https://github.com/php/php-src/commit/72e1da81b6792d4141b4b41f6fc3e92b68aa0f3e

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

Looking at Debian.. It only affects >= buster

https://packages.debian.org/stretch/curl 7.52
https://packages.debian.org/buster/curl 7.64

Reedy moved this task from Backlog to MediaWiki core on the PHP 7.4 support board.Oct 6 2020, 2:24 PM
Reedy moved this task from Backlog to MediaWiki core on the PHP 8.0 support board.
Ciencia_Al_Poder added a subscriber: Ciencia_Al_Poder.Oct 6 2020, 2:56 PM
Reedy added a project: MW-1.35-release.Oct 6 2020, 3:16 PM
Reedy moved this task from Blocker to Non blocker on the MW-1.35-release board.
Jdforrester-WMF added a subscriber: Jdforrester-WMF.Oct 6 2020, 7:44 PM
CCicalese_WMF edited projects, added Platform Engineering; removed Platform Team Workboards.Oct 25 2020, 11:32 AM
• AMooney triaged this task as Medium priority.Nov 10 2020, 9:29 PM
• AMooney edited projects, added Platform Team Workboards (Clinic Duty Team); removed Platform Engineering.
• AMooney moved this task from Inbox to Later on the Platform Team Workboards (Clinic Duty Team) board.
Ammarpad mentioned this in T264986: Check that curl is new enough to support CURLMOPT_MAX_HOST_CONNECTIONS.Nov 11 2020, 6:21 AM
gerritbot added a comment.Dec 2 2020, 1:27 AM

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

https://gerrit.wikimedia.org/r/644640

gerritbot added a project: Patch-For-Review.Dec 2 2020, 1:27 AM
tstarling added a subscriber: tstarling.Dec 2 2020, 10:09 PM

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.

tstarling added a comment.Dec 2 2020, 10:20 PM

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.

IijimaYun added a comment.Dec 3 2020, 7:34 PM

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).

aaron mentioned this in T250417: Deprecate and remove SquidPurgeClient classes.Jan 4 2021, 10:59 PM
Prod added a subscriber: Prod.Feb 1 2021, 10:01 AM
Reedy merged a task: T275064: CURLPIPE_HTTP1 is no longer supported in MultiHttpClient.php.Feb 17 2021, 9:18 PM
Reedy added a subscriber: Justin_C_Lloyd.
Krinkle added a project: MW-1.36-release.Feb 20 2021, 12:02 AM
Krinkle added a subscriber: Krinkle.EditedFeb 20 2021, 12:05 AM

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.

Krinkle mentioned this in T39063: SquidPurgeClient hangs on response from Squid Cache 2.7, or multiple entries in $wgSquidServers.Apr 2 2021, 8:11 PM
Lens0021 added a subscriber: Lens0021.Apr 10 2021, 4:14 PM
Reedy moved this task from Blocker to Not a blocker on the MW-1.36-release board.Apr 15 2021, 6:13 PM
Pneuma01 added a subscriber: Pneuma01.May 18 2021, 1:58 PM
Octfx added a subscriber: Octfx.Jun 2 2021, 8:40 PM

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

Lens0021 awarded a token.Jun 16 2021, 4:53 AM
gerritbot added a comment.Mon, Sep 13, 10:12 PM

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

[mediawiki/core@master] Remove deprecated CURLPIPE_HTTP1

https://gerrit.wikimedia.org/r/720704

Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL