Page MenuHomePhabricator

Support callback functions in GuzzleHttpRequest and MultiHttpClient
Closed, ResolvedPublic

Description

Recent changes to use the Guzzle library inadvertently broke http-related code that uses callbacks to process http data. These errors have been temporarily circumvented by switching back to our old CurlHttpRequest request class instead of the new Guzzle one, and the production site is currently happy. But we need a long-term solution.

Guzzle was recently introduced as a vendor library, in T202110. This task also set the new GuzzleHttpRequest as the default MWHttpRequest-derived class used by HttpRequestFactory. Guzzle has many advantages, including automatic fallback to php streams if curl is not available, which will allow us to make mediawiki not depend on curl. Guzzle also supports a modern, powerful, and flexible "sink" system for processing http data in PSR-7 streams, which might be a good replacement for our current callback functions:
https://guzzle.readthedocs.io/en/latest/request-options.html#sink
https://github.com/guzzle/psr7#psr-7-message-implementation
However, Guzzle does not (directly) support passing a callback function in the way our CurlHttpRequest class does.

The initial implementation released under T202110 supported the Guzzle "sink" option, and related use of PSR-7 streams. However, the task implementor (full disclosure: me) neither updated existing code to use the "sink" option, nor added a compatibility layer to keep existing code functioning without error, resulting in the issues seen in T211886.

The simplest and most flexible solution appears to be to add compatible callback support to GuzzleHttpRequest (and MultiHttpClient before it is released in T202352).

For GuzzleHttpRequest, it appears this can be readily accomplished by creating a custom stream class and overriding the write() function to invoke the callback function provided via the "callback" parameter. Existing callers should not require any changes. This technique is similar to https://github.com/guzzle/psr7#implementing-stream-decorators

We should also confirm that MultiHttpClient is unaffected. Its comparable parameter, "stream", is similar to Guzzle's approach, so this may just be verification of that parameter's functionality under Guzzle.

If possible, unit testing should confirm that all this works as desired so that any related future issues are automatically detected.

Event Timeline

BPirkle created this task.Dec 17 2018, 11:53 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 17 2018, 11:53 PM
CCicalese_WMF triaged this task as High priority.Dec 18 2018, 3:49 PM

Change 481954 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Support callback functions in GuzzleHttpRequest

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

Change 481954 merged by jenkins-bot:
[mediawiki/core@master] http: Support callback functions in GuzzleHttpRequest

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

BPirkle closed this task as Resolved.Jan 25 2019, 2:39 PM

Patch merged, this is complete

BPirkle updated the task description. (Show Details)Feb 5 2019, 1:50 AM