[8.x] Return a new or existing guzzle client based on context

[craigballinger]

Issue

This PR resolves a breaking change in backwards compatibility between Laravel Framework 8.36.2 and 8.37. Laravel 8.37 introduced support for concurrent asynchronous requests in the HTTP client #36948. As part of that change, a protected $client property was introduced in the PendingRequest class which stores a reference to the Guzzle client in order to reuse it for asynchronous request pooling. An unintended side effect of this update occurs when making synchronous calls through the HTTP client. The reused client appears to hold references to every request made in a script, keeping all streams (temporary files) open until the script completes. This results in a "too many open files" error when sufficient HTTP requests are made in a process #38641. In our case that number was ~300 running in a queued job.

Proposed Solution

This update restores the previous functionality of returning a new Guzzle client for each standard HTTP request. In the updated buildClient method, the PendingRequest will check if either a Guzzle client has been set using the setClient method, or if the call is asynchronous ($this->async), if so, it will continue to use the $this->client property. If not, it will return a new client as it had previously. The change supports the new pooling feature without breaking backwards compatibility.

Further Development

It does seem the appearance of this issue hints at a further refactoring that may be necessary at some point. While the async calls don't seem to create the same open files issue, manually setting a Guzzle client using the setClient method may still be affected by the underlying problem depending on configuration of that client. I suspect the source of the problem may be that open references to Guzzle Request objects in the reused Guzzle Client are preventing them from closing their streams and being garbage collected as the requests in the recreated client are when it goes out of scope, but I am uncertain.

[driesvints]

@craigballinger please add a thorough explanation to your PR why this is needed and don't link to an issue (see the PR template when submitting PR's).

[craigballinger]

Apologies, I just went looking for a template, but I guess it's only in the original PR submission? I'll do my best to update the original PR comment to follow what other active PRs cover.

[driesvints]

This is what you should see when submitting new PR's. Doesn't that show up for you?

[craigballinger]

I saw it when I did the original submission, but wasn't able to pull it up again this morning when editing that comment or submitting a new one. Looks like the requirements should be covered in the updated original comment now, minus the tests, but the existing tests covered the functionality that is being restored. If you need anything else, please let me know. Thanks!

[driesvints]

Thanks @craigballinger!