Skip to content

[HttpClient] Fix closing curl-multi handle too early on destruct #44601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #44334
License MIT
Doc PR -

For some reason, the garbage collector can decide to destruct the CurlClientState before the responses that reference them.
When this happens, the curl-multi handle is closed and responses end up in a broken state.
This fixes it by not closing the multi-handle on destruct/reset.

This also fixes configuring the multi-handle on reset.

@stof
Copy link
Member

stof commented Dec 13, 2021

is it possible to have tests to prevent such regressions or is it too tricky to trigger ?

@nicolas-grekas
Copy link
Member Author

It's too tricky, PHP engine closing in unpredictable order...

@nicolas-grekas nicolas-grekas merged commit 1e3d60b into symfony:4.4 Dec 13, 2021
@nicolas-grekas nicolas-grekas deleted the hc-destruct branch December 13, 2021 17:56
nicolas-grekas added a commit that referenced this pull request Dec 14, 2021
…) (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] fix monitoring responses issued before reset()

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Follow up of #44601

In order to reset the DNS cache, we have to create a new curl-multi handle. But since there might still be responses attached to the previous handle, we need to keep it around to monitor it.

Best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/44625/files?w=1).

Commits
-------

6e52960 [HttpClient] fix monitoring responses issued before reset()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants