Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[HttpClient] Fix CurlHttpClient memory leak #38493

Merged
merged 1 commit into from Oct 10, 2020
Merged

[HttpClient] Fix CurlHttpClient memory leak #38493

merged 1 commit into from Oct 10, 2020

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Oct 8, 2020

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

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Oct 9, 2020
@Nyholm
Copy link
Member

Nyholm commented Oct 9, 2020

FYI: I tested this PR, it does fix the memory leak.

@fabpot
Copy link
Member

fabpot commented Oct 10, 2020

Thank you @HypeMC.

@fabpot fabpot merged commit 1cef665 into symfony:4.4 Oct 10, 2020
@HypeMC HypeMC deleted the fix-curlhttpclient-memory-leak branch October 10, 2020 15:33
@HypeMC HypeMC mentioned this pull request Oct 10, 2020
Merged
nicolas-grekas added a commit that referenced this pull request Oct 11, 2020
@nicolas-grekas
minor #38508 [HttpClient] Content doesn't get decoded when fetched fr…
a8479e8 
…om an exception (HypeMC)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] Content doesn't get decoded when fetched from an exception

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

My previous PR #38493 created a new bug, the content doesn't get decoded when fetched from an exception because the inflate resource gets unset:

```php
<?php

use Symfony\Component\HttpClient\CurlHttpClient;
use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface;

include __DIR__.'/symfony/vendor/autoload.php';

$client = new CurlHttpClient();

try {
    $client->request('GET', 'http://example.com/404');
} catch (ClientExceptionInterface $exception) {
    echo $exception->getResponse()->getContent(false);
}
```

I've removed the part where the resource gets unset since it still might be used at some point.

The test is implementation independent so I believe it should go in contracts, please correct me if I wrong. Also, I was unable to find a way to do the test without adding a new endpoint this time, any suggestions would be appreciated.

Commits
-------

8fa4f85 Don't unset the inflate resource on close as it might still be needed
@fabpot fabpot mentioned this pull request Oct 14, 2020
Merged
This was referenced Oct 28, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Bug HttpClient Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@HypeMC @Nyholm @fabpot @nicolas-grekas @stof @derrabus @carsonbot