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

[5.8] Allocate memory for error handling to allow handling memory exhaustion limits #29226

Merged
merged 2 commits into from Jul 21, 2019

Conversation

@skylerfenn
Copy link
Contributor

commented Jul 18, 2019

Allots 10kb of memory for "memory exhaustion" errors.

It lets the application give a more descriptive error than the default Fatal Error that is thrown for memory exhaustion.

The concept was taken from Symfony's error handler: https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/ErrorHandler/ErrorHandler.php.

skylerfenn added some commits Jul 18, 2019

@driesvints driesvints changed the title Allocate memory for error handling to allow handling memory exhaustion limits [5.8] Allocate memory for error handling to allow handling memory exhaustion limits Jul 19, 2019

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Provide before and after screenshots of the difference in error messages please.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

This solution won't work, however, in the case that this method is called more than once?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Is setting to null actually guaranteed to release the memory. How does PHP calculate this? Do we have to wait for the garbage collector?

@laurencei

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Also - whats the performance hit (if any) on running the 'x' repeat command 10240 times? Both in terms of time and memory?

@skylerfenn

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

@taylorotwell
Current Error
current error

Better Error
better error

@GrahamCampbell
The bootstrap method will only run each bootstrapper once on a request:
https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Http/Kernel.php#L161

From what I've been able to see, setting a variable to null frees the memory immediately.

@laurencei
The function is only running once with 1 byte. The PHP source for str_repeat shows: "Heavy optimization for situations where input string is 1 byte long". Do you know of a more efficient way to allocate 10kb? Again, this idea was taken from Symfony's error handler. Throughout the history of that project, I couldn't find any discussion on this.

Thanks for the feedback and comments!

@mfn

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

I like this approach, very useful.

Though I would find it really nice if there was a bit of documentation on the proper on what it is for.

@skylerfenn do you think you could add that?

@taylorotwell taylorotwell merged commit 805450a into laravel:5.8 Jul 21, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cxlblm

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Memory is very expensive. Is that really reasonable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.