Skip to content

Memory leak hotfix #184

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

Closed
wants to merge 5 commits into from

Conversation

thomasvargiu
Copy link

Hi,
there is a big memory leak when using the PluginClient with plugins.

Example code to reproduce the issue:

use Http\Client\Curl\Client;

$samples = 50;
$curlClient = new Client();
$requestFactory = $container->get(\Psr\Http\Message\RequestFactoryInterface::class);
$pluginClient = new \Http\Client\Common\PluginClient(
    $curlClient,
    [
        new \Http\Client\Common\Plugin\RedirectPlugin(),
    ]
);

$request = $requestFactory->createRequest('GET', 'https://google.it');
for ($i = 1; $i <= $samples; $i++) {
    $pluginClient->sendRequest($request);
    echo 'memory: ' . memory_get_usage() . PHP_EOL;
    // gc_collect_cycles(); without this, gc isn't able to free memory
}

This PR fixes it removing useless variable reference.

@GrahamCampbell
Copy link
Contributor

Tests are failing now?

@thomasvargiu
Copy link
Author

Yes you're right, what I wrote is breaking the chain. I'm going to look at it better

@thomasvargiu
Copy link
Author

thomasvargiu commented Feb 16, 2020

So, the problem is on passing the first callable to plugins, it creates a reference loop.
The only way to resolve it, is to create a new chain of callables for every plugin request, using the new PluginChain instance as first callable.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks reasonable to me.

@Nyholm do you agree, or are you worried about side effects or some other overhead resulting from this?

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really call the plugins in correct order?

I might be misreading something, but it seams like the order is reversed now. Do we have tests for the order plugins are called?

Copy link
Contributor

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the CS fixes, then rebase.

@dbu
Copy link
Contributor

dbu commented Jul 1, 2020

Please revert the CS fixes, then rebase.

@GrahamCampbell is the change with the plugin chain good otherwise?

@GrahamCampbell
Copy link
Contributor

Yeh, if you extract just the changes to the files that are needed (I think 3 files?).

@GrahamCampbell
Copy link
Contributor

Sorted out a replacement here: #194.

@dbu dbu closed this in #194 Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants