Skip to content

Fix GH-17951: Addition of max_memory_limit INI #18011

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

frederikpyt
Copy link

@frederikpyt frederikpyt commented Mar 10, 2025

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
Copy link
Member

Looks good in general though. 👍

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise, thank you!

@iluuu1994
Copy link
Member

@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
Copy link
Member

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

@iluuu1994
Copy link
Member

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.

if (PG(max_memory_limit) != -1) {
if (value == -1) {
zend_error(
stage == ZEND_INI_STAGE_STARTUP ? E_ERROR : E_WARNING,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iluuu1994
Copy link
Member

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
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

5 participants