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

MEMORY_LIMIT not observed for AbstractHydrator.php #2201

Closed
theofficialgman opened this issue Oct 3, 2024 · 5 comments · Fixed by #2203
Closed

MEMORY_LIMIT not observed for AbstractHydrator.php #2201

theofficialgman opened this issue Oct 3, 2024 · 5 comments · Fixed by #2203
Labels
Milestone

Comments

@theofficialgman
Copy link

Shlink version

4.2.0

PHP version

8.3.11

How do you serve Shlink

Self-hosted nginx

Database engine

MySQL

Database version

8.0.39

Current behavior

Memory limit is set as MEMORY_LIMIT' => '768M' however this is not observed by shlink for all functions and some still error with a memory limit of 512 mebibytes

PHP Fatal error:  Allowed memory size of 536870912 bytes exhausted (tried to allocate 20480 bytes) in /var/www/shlink/vendor/doctrine/orm/src/Internal/Hydration/AbstractHydrator.php on line 321

Expected behavior

I expect that the MEMORY_LIMIT applies to all functions of shlink

Minimum steps to reproduce

  1. set MEMORY_LIMIT to a value above the default
  2. run a command with large output to cause memory exhaustion
shlink-cli visit:non-orphan
  1. observe the exhausted memory size is not the configured MEMORY_LIMIT but instead 536870912 bytes (which is exactly 512 mebibytes)
@acelaya
Copy link
Member

acelaya commented Oct 3, 2024

The steps to reproduce do not indicate how exactly you are setting the env var.

Did you set it via Shlink installer?

@theofficialgman
Copy link
Author

Did you set it via Shlink installer?

Yes I did.

@acelaya
Copy link
Member

acelaya commented Oct 3, 2024

Hmmm, yeah, I think I know what's going on.

Tl;dr

For simplicity, Shlink "promotes" config options as env vars, that way they can be handled the same once the app is bootstrapped.

However, config options are promoted as env vars at one specific point, but this particular one is being read before the config is loaded at all, making it fall back to the default value.

Specifically here

ini_set('memory_limit', EnvVars::MEMORY_LIMIT->loadFromEnv('512M'));
, and configuration is loaded a few lines after.

For people providing the options as actual env vars this would not happen, but I didn't realize about this particular scenario.

I see some possible short term/temporary solutions:

  1. For your specific setup, the simplest approach is to set the memory_limit in your php.ini config. As you can see, that's basically what Shlink does when this env var is read.
  2. Another option would be to run your command as MEMORY_LIMIT=768M shlink-cli visit:non-orphan, but you would have to do that for every command, and it would not affect HTTP requests, so I'd recommend going with option 1 over this one.
  3. Setting MEMORY_LIMIT as an actual env var in your system.

The long term solution would involve making Shlink set the option again after the config has been read, but anything executed before that point would not be affected. I think that's negligible.

@acelaya acelaya moved this to Todo in Shlink Oct 3, 2024
@acelaya acelaya added this to the 4.2.1 milestone Oct 3, 2024
@theofficialgman
Copy link
Author

Thanks for the solutions.

For now I went ahead and implemented a function of my script (that is already wrapping around shlink-cli to obtain the information I am interested in) to obtain data weekly in a loop from the start to end date to workaround this issue. It is really better that way for me anyway as the host system (a remote micro server) only has 1GB of ram in the first place.

@acelaya
Copy link
Member

acelaya commented Oct 4, 2024

I have just released v4.2.1, which includes a fix for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants