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.

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

[BUG] Memory leak with async/future mode on 2.4.1 ? #297

Closed
romainruaud opened this issue Feb 14, 2025 · 7 comments
Closed

[BUG] Memory leak with async/future mode on 2.4.1 ? #297

romainruaud opened this issue Feb 14, 2025 · 7 comments
Labels
bug Something isn't working

Comments

@romainruaud
Copy link
Contributor

romainruaud commented Feb 14, 2025

What is the bug?

We used to rely on async/future mode for our indexing tasks, because it used to have best performances overall.

The syntax we used was this one (generic logic can be read on this class as well, but it's basically storing the bulks and resolving them all-at-once by batch instead of sending them sequentially) :

https://github.com/Smile-SA/elasticsuite/blob/2.11.x/src/module-elasticsuite-core/Index/AsyncIndexOperation.php#L96

This worked well for ages but it's now producing OOM errors according to our users, and rollbacking the opensearch php client to 2.3.1 fixes the issue. See Smile-SA/elasticsuite#3522 (comment)

If I trace the execution on my laptop, the behavior is the following :

  • with 2.3.1 client : memory usage remains the same during all the indexing process.
  • with 2.4.1 client : memory usage is continuously raising. Total indexing time is 2x more than with 2.3.1 client.

If I don't use the "async" mode and send each bulk one by one, memory usage does not raise.

So I suspect that something is not properly cleared when performing requests in async mode with >2.4.0 client, and remains in memory, increasing the footprint.

For the sake of your investigations, here is our "Client" class : https://github.com/Smile-SA/elasticsuite/blob/2.11.x/src/module-elasticsuite-core/Client/Client.php

And our ClientBuilder : https://github.com/Smile-SA/elasticsuite/blob/2.11.x/src/module-elasticsuite-core/Client/ClientBuilder.php

Potential workaround : we can renew the Client object after processing our bulks, but the memory keeps increasing until we renew it. And in the end, the indexing time is still way higher than before. So far this is not ideal.

I'll be glad to provide any additional informations.

@romainruaud romainruaud added bug Something isn't working untriaged labels Feb 14, 2025
@dblock
Copy link
Member

dblock commented Feb 14, 2025

This is not good :( I would try to write the smallest repro possible against a local docker pulled OpenSearch, then profile the app to see what is not being cleaned up.

@dblock dblock removed the untriaged label Feb 14, 2025
@romainruaud
Copy link
Contributor Author

Hi @dblock , if this can help : https://github.com/romainruaud/opensearch-client-memory-flaw

docker compose build
docker compose run --rm php-app-231 composer install && docker compose run --rm php-app-231 php benchmark.php
docker compose run --rm php-app-241 composer install && docker compose run --rm php-app-241 php benchmark.php

The 2.3.1 version runs in ~6secs on my laptop, and the 2.4.1 runs in ~11secs so definitely there is a huge discrepancy.

I did not profile for now.

Regards

@romainruaud
Copy link
Contributor Author

In 2.4.1 client, $options is always empty here : https://github.com/opensearch-project/opensearch-php/blob/2.4.1/src/OpenSearch/Transport.php#L155

So most likely requests are not sent async even when it's clearly asked in the client options. So they are probably stockpiling in the RAM :(

@romainruaud
Copy link
Contributor Author

Ok, turns out it comes from this line : https://github.com/opensearch-project/opensearch-php/blame/main/src/OpenSearch/LegacyTransportWrapper.php#L35

calling wait() explicitely causes the promise to be resolved even when in future mode. Not cool :(

Gonna open a PR soon

romainruaud added a commit to romainruaud/opensearch-php that referenced this issue Feb 17, 2025
romainruaud added a commit to romainruaud/opensearch-php that referenced this issue Feb 17, 2025
Signed-off-by: Romain Ruaud <[email protected]>
romainruaud added a commit to romainruaud/opensearch-php that referenced this issue Feb 17, 2025
Signed-off-by: Romain Ruaud <[email protected]>
@kimpepper
Copy link
Collaborator

I've expanded on #299 and added additional tests in #300

@kimpepper
Copy link
Collaborator

I still think we need to work out why there is a memory leak with synchronous mode.

@kimpepper
Copy link
Collaborator

I think we can close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants