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

[Jeroeny]

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]

Hey!

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

Cheers!

Carsonbot

[nicolas-grekas]

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 :) )

[Jeroeny]

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]

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]

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]

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]

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 👍