Fix GH-17951: Addition of max_memory_limit INI

[frederikpyt]

Added a new INI max_memory_limit which acts as a cap on how much you can raise memory_limit using ini_set() during runtime.

Perhaps see: #17951

[iluuu1994]

Looks good in general though. 👍

[iluuu1994]

LGTM otherwise, thank you!

[iluuu1994]

@frederikpyt Feel free to ping me again ~2 weeks after you've sent the e-mail to the internals mailing list. If there are no objections, this may be merged.

[nielsdos]

This feature looks interesting, but I don't see the email to the internals mailing list?

[iluuu1994]

Looks like it was never sent, unfortunately. @frederikpyt Are you still planning on sending an e-mail? It doesn't need to be long, but it would be a shame if your work went to waste.

[iluuu1994]

Looking at this again, I think it would be better to stick to a warning in all cases. Erroring is very harsh, especially on startup, taking down the entire website.

[frederikpyt]

I'll change all cases to a warning :)

[iluuu1994]

One open question is whether exceeding the max_memory_limit should set memory_limit to max_memory_limit, giving it a higher chance not to straight out error.

I have sent a short e-mail to internals now (will add the link once it's available in externals.io).

[TimWolla]

I have sent a short e-mail to internals now (will add the link once it's available in externals.io).

It appears you forgot, here it is: https://externals.io/message/127108

[frederikpyt]

Looks like it was never sent, unfortunately. @frederikpyt Are you still planning on sending an e-mail? It doesn't need to be long, but it would be a shame if your work went to waste.

Just got back from easter vacation. 😄 Thanks @iluuu1994 for sending the internals email, I completely forgot to check up on it.

One open question is whether exceeding the max_memory_limit should set memory_limit to max_memory_limit, giving it a higher chance not to straight out error.

I think that it'd be best to try and raise the memory limit as much as possible to reduce risk of OOM.

But as far as I can tell it'd require a larger change, refactoring the ini_set() method. The C code currently doesn't support setting INI's to something different than the argument passed through PHP. You can only set it to the passed value or fail and not change it at all.

[iluuu1994]

Sorry for the late response. We got only one feedback from the mailing list (apart from a +1):

I think that it'd be best to try and raise the memory limit as much as possible to reduce risk of OOM.

Especially when setting memory_limit to -1 you'd definitely want this to mean "use the max allowed memory limit" and perhaps even skip the warning in that case as it's not a very specific request for a memory amount.

So, it seems the desired behavior would be:

• When trying to set memory_limit something higher than max_memory_limit, warn and set it to the same value as max_memory_limit.
• When trying to set memory_limit to -1 while max_memory_limit is set, set it to max_memory_limit. Skip the warning.

Can you try to make the necessary changes? If not (for whatever reason, you're in no obligation ofc), I can have a look. Thanks!

[frederikpyt]

Hi @iluuu1994,

I agree 100% on the desired behaviour you've described.

It's actually the behaviour I tried to implement at first, however I stumbled into a larger issue. Beware that I'm not experience in C at all and this is actually my first C code ever, so that may be part of the issue. 😁

When attempting to modify an INI setting to a value different from what is set via ini_set() (e.g., setting memory_limit to -1 while max_memory_limit is set to 100M), I encountered an issue where PHP's internal values appeared to reflect the new memory_limit setting correctly, but ini_get() still returned the previous value.

Perhaps it was because i returned FAILURE instead of SUCCESS in PHP_INI_MH(OnChangeMemoryLimit) 🤷🏻

I currently lack the time and feel I have insufficient C knowledge, so I’d appreciate it if you could take a look.

Thanks for all the help this far :)