Page MenuHomePhabricator

MultiHttpClient has hard-coded deprecated cURL option used by CdnCacheUpdate
Closed, ResolvedPublic

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?

Event Timeline

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

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

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

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

For the record, we've been using curl in production with the dead code for one year, but given that curl now defaults to http2 when available it probably didn't matter.

I would propose we simply remove the option or make it dead code for now if we want a deprecation of the class interface.

Actually, I just realized we now handle purges in production exclusively via purged so we just enqueue purge messages on the relevant kafka topics now, see

https://gerrit.wikimedia.org/g/operations/mediawiki-config/+/e8b8a9c7231e276a51bf18911c22086b5d1e9b15/wmf-config/InitialiseSettings.php#25509

If it is possible to move the current functionality into an extension without losing performance, it would be rather useful since it would be a nice example in ‘how to make a purge client extension’. I wanted to implement a small purge proxy in Rust but got stuck in MW's purge client code and then kind of forgot about it.

If it is possible to move the current functionality into an extension without losing performance, it would be rather useful since it would be a nice example in ‘how to make a purge client extension’. I wanted to implement a small purge proxy in Rust but got stuck in MW's purge client code and then kind of forgot about it.

Based on https://phabricator.wikimedia.org/T216225#5335375, the sanest way of doing that would probably be factoring out the HTCP and HTTP purge logic that currently lives in CdnCacheUpdate into one or two EventRelayer implementations with some B/C handling for the existing config options that enable those.

Change 756750 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Fix deprecation warning from CURLPIPE_HTTP1

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

Change 720704 abandoned by Reedy:

[mediawiki/core@master] Remove deprecated CURLPIPE_HTTP1

Reason:

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/756750

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

Change 644640 abandoned by Reedy:

[mediawiki/core@master] Stop setting CURLMOPT_PIPELINING on curl >= 7.62.0

Reason:

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/756750

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

Change 756750 merged by jenkins-bot:

[mediawiki/core@master] Fix deprecation warning from CURLPIPE_HTTP1

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

Change 757028 had a related patch set uploaded (by Jforrester; author: Tim Starling):

[mediawiki/core@REL1_37] Fix deprecation warning from CURLPIPE_HTTP1

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

Change 757029 had a related patch set uploaded (by Jforrester; author: Tim Starling):

[mediawiki/core@REL1_36] Fix deprecation warning from CURLPIPE_HTTP1

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

Change 757030 had a related patch set uploaded (by Jforrester; author: Tim Starling):

[mediawiki/core@REL1_35] Fix deprecation warning from CURLPIPE_HTTP1

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

Change 757030 merged by jenkins-bot:

[mediawiki/core@REL1_35] Fix deprecation warning from CURLPIPE_HTTP1

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

Change 757029 merged by jenkins-bot:

[mediawiki/core@REL1_36] Fix deprecation warning from CURLPIPE_HTTP1

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

Change 757028 merged by jenkins-bot:

[mediawiki/core@REL1_37] Fix deprecation warning from CURLPIPE_HTTP1

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

Just CdnCacheUpdate to be updated now to finish this off?

I think a rewrite of CdnCacheUpdate is not part of this task. Third party users can just follow WMF and use HTCP if they want performance. It's unclear whether anyone actually needs or wants a rewrite of CdnCacheUpdate.

I assumed HTCP is no longer used by WMF?

Multicast HTCP purging was the dominant method of purging Varnish/ATS HTTP cache objects in our Traffic infrastructure until July 2020, which is when we switched to Kafka-based CDN purging.

https://wikitech.wikimedia.org/wiki/Multicast_HTCP_purging