Skip to content

[HttpClient] Fix memory leak in TraceableHttpClient for environments with profiling #43287

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
Closed
wants to merge 1 commit into from
Closed

[HttpClient] Fix memory leak in TraceableHttpClient for environments with profiling #43287

wants to merge 1 commit into from

Conversation

Jeroeny
Copy link
Contributor

@Jeroeny Jeroeny commented Oct 2, 2021 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets See description in this PR
License MIT

Each DataCollector implementation usually holds the Traceable versions of certain interfaces.
These are used by the profiler to display the traces of calls. For long running processes, this can lead to increased memory usage over time. To prevent memory leakage, the reset() method can be called. For example on the profiler service, which holds services tagged with data_collector. Which is used by the new Messenger reset feature as well.

On reset, each DataCollector removes it's data from memory, and that of the Traceable classes that it holds. See for example the EventDataCollector.

Now the HttpClientDataCollector seems to skip resetting the data of the TraceableHttpClient(s) that it holds. This DataCollector only resets its own data. There doesn't seem to be any other class/service calling reset on the TraceableHttpClient. Therefor it should probably be done by this DataCollector? Unless there is a specific reason this is skipped, that I'm not aware about that.

I've tested this setup to confirm that without the reset on the clients, the memory keeps increasing, and with the reset being called, the memory no longer increases.

Something else I found curious but might not be related:

bin/console debug:container http_client


 // This service is a private alias for the service .debug.http_client

Information for Service ".debug.http_client"
============================================

 ---------------- --------------------------------------------------
  Option           Value                                            
 ---------------- --------------------------------------------------
  Service ID       .debug.http_client
  Class            Symfony\Component\HttpClient\TraceableHttpClient
  Tags             monolog.logger (channel: http_client)
                   http_client.client
...

Note the tag http_client.client being present.

Now listing services by that tags gives none:

bin/console debug:container --tag http_client.client

Symfony Container Services Tagged with "http_client.client" Tag
===============================================================

 ------------ ------------
  Service ID   Class name 
 ------------ ------------

Though the client is injected into data_collector.http_client, which searches services on that tag as well.

@carsonbot carsonbot added this to the 5.4 milestone Oct 2, 2021
@Jeroeny Jeroeny changed the base branch from 5.4 to 4.4 October 2, 2021 10:25
@carsonbot
Copy link

Hey!

I think @fancyweb has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 4.4 Oct 6, 2021
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 6, 2021
Merged
@nicolas-grekas
Copy link
Member

Hi @Jeroeny and thanks for the PR.
I submitted #43333 instead because I think this would be a more appropriate fix.
Can you please confirm that it also fixes the issue you reported?
About the commands, that's another topic (not a critical one I would say :) )

fabpot added a commit that referenced this pull request Oct 6, 2021
@fabpot
bug #43333 [HttpClient] fix missing kernel.reset tag on TraceableHttp…
e591467 
…Client services (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] fix missing kernel.reset tag on TraceableHttpClient services

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

Replaces #43287

Commits
-------

2951f53 [HttpClient] fix missing kernel.reset tag on TraceableHttpClient services
@Jeroeny
Copy link
Contributor Author

Jeroeny commented Oct 6, 2021 •
edited
Loading

I submitted #43333 instead because I think this would be a more appropriate fix. Can you please confirm that it also fixes the issue you reported?

Thanks for the help. It seems that with that tag, the TraceableHttpClient is not reset when calling reset on the profiler service, unfortunately. Is the client expected to be reset when the profiler is reset?

@nicolas-grekas
Copy link
Member

TraceableHttpClient is not reset when calling reset on the profiler

That's correct, but what you want is that TraceableHttpClient is reset when calling reset on the container, which what the tag should do, isn't it?

@Jeroeny
Copy link
Contributor Author

Jeroeny commented Oct 6, 2021

That's correct, but what you want is that TraceableHttpClient is reset when calling reset on the container, which what the tag should do, isn't it?

The kernel.reset tag indeed behaves as expected. I'd have liked a way to reset it as part of the DataCollectors, because then I can specifically reset profiling data. With a container reset, things like ArrayAdapter caches are also reset. But I just noticed that the messenger reset feature also resets services of kernel.reset, so it's probably intended this way?

@nicolas-grekas
Copy link
Member

I'd have liked a way to reset it as part of the DataCollectors, because then I can specifically reset profiling data

is that a real world use case? as of now, resetting is supposed to be managed by the container, as resetting only a subset of leaking services makes little sense.

@Jeroeny
Copy link
Contributor Author

Jeroeny commented Oct 6, 2021

is that a real world use case? as of now, resetting is supposed to be managed by the container, as resetting only a subset of leaking services makes little sense.

It was for debugging a production memory leak, where we'd want to reset the profilers and leave only the memory leaks that shouldn't be there. So that we could better evaluate the production-level leak (which could for example be in a misconfigured ArrayAdapter without a $maxItems).

Though I agree it's a bit of a specific use case. As far as I'm concerned the current setup is fine then, as we have enough options to approach this problem (like running a local environment in APP_ENV=prod). Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr Awaiting requested review from chalasr

@wouterj wouterj Awaiting requested review from wouterj

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

Successfully merging this pull request may close these issues.
3 participants
@Jeroeny @carsonbot @nicolas-grekas