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

PHP Fatal error: Allowed Memory Size #226

Open
aleemb opened this issue Apr 2, 2017 · 27 comments
Open

PHP Fatal error: Allowed Memory Size #226

aleemb opened this issue Apr 2, 2017 · 27 comments
Labels
needs discussion Requires internal analysis/discussion

Comments

@aleemb
Copy link

aleemb commented Apr 2, 2017

On a few occasions where I imagine the stack trace is large, BugsnagLogger is unable to log the error and fails with PHP Fatal error: Allowed Memory Size. This means the original error is lost for good since it isn't logger and the PHP error logs don't end up showing this error either as the request fails fatally.

Some kind of check or config option to circumvent this would be very helpful.

@GrahamCampbell
Copy link
Contributor

Thank you for the report. That is very interesting. Out of interest, what is your PHP memory limit, and are you certain that this is caused by Bugsnag, and not Bugsnag trying to catch the out of memory error, caused by your app?

@aleemb
Copy link
Author

aleemb commented Apr 4, 2017

Some more details. If I disable BugSnag the error dumps to the screen just fine. However, If I enable BugSnag, I see the following in the logs

2017/04/03 21:22:23 [error] 58#0: *48368 FastCGI sent in stderr: "PHP message: PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 262115 bytes) in /var/www/foobar/vendor/bugsnag/bugsnag-psr-logger/src/BugsnagLogger.php on line 48" while reading response header from upstream, client: 192.168.64.1, server: foobar.com, request: "POST /admin/form HTTP/1.1", upstream: "fastcgi://unix:/var/run/php/php5.6-fpm.sock:", host: "foobar.com", referrer: "https://foobar.com/admin/form"

and the browser default 500 error page

image

@aleemb
Copy link
Author

aleemb commented Apr 4, 2017

I am on v2.4.0 of bugsnag/bugsnag-laravel which is using v3.4.0 of bugsnag/bugsnag

@dominics
Copy link

dominics commented Apr 7, 2017

See also #19 etc.

are you certain that this is caused by Bugsnag, and not Bugsnag trying to catch the out of memory error, caused by your app

Both of those cases are problematic (and my logs indicate both occur).

The Bugsnag client allocates significant amounts of memory in many places. Everything from the static array in ErrorTypes allocating 32k, to the json_encode() in HttpClient using essentially unbound memory (I've seen megabytes there).

So, both cases happen:

  • sometimes an error just coincidentally happens when memory usage is near the limit and, were it not for Bugsnag's allocations, the error would have been otherwise handled normally
  • sometimes something else in the app runs out of memory, and BugSnag tries to report the error and then hits a second OOM error

Both result in nothing in Bugsnag. Obviously the first case is purely caused by BugSnag, but the second is pretty poor form too.

In fact, unless the Bugsnag client checks memory_get_usage(), and optionally issues an ini_set() (to account for the extra memory that's about to be allocated), I don't see how comments like #19 (comment) ("looks like bugsnag-php should be able to catch memory errors") can be correct...

Maybe PHP used to disable the memory limit after the first time it hit it? It certainly doesn't any longer.

@GrahamCampbell
Copy link
Contributor

It is not possible to catch out of memory errors:

image

Attempting to do anything, such as raising the memory limit (which would probably be a bad idea), will use more memory, and also run out of memory.

@dominics
Copy link

dominics commented Apr 9, 2017

@GrahamCampbell A couple of notes:

It's not a good idea to test this bug with a recursive stack overflow. That's because xdebug provides a stack limit that on normal PHP instances will hit much earlier (256 recursions by default?) than a memory limit. So you run the risk of confusing people trying to reproduce in their dev environments. I suggest $v = str_repeat(' ', PHP_MAX_INT); (it's also quicker to hit the limit given).

Next, check out this 3v4l: https://3v4l.org/qIH7Z

register_shutdown_function(function () {
    var_dump(memory_get_usage());
    var_dump(error_get_last());
    
    $lotsOfMemoryLeftThatCanBeUsed = str_repeat(' ', 1000);
    var_dump(strlen($lotsOfMemoryLeftThatCanBeUsed));
});

$v = str_repeat(' ', PHP_INT_MAX);

This works, indicating you can catch memory errors (just not with the exception/error handler). Furthermore, if the allocation that failed was large, you'll have plenty of available memory to process the error.

Even where the OOM is caused by small allocations, there are still approaches (albeit hacky ones) that can deal the error (by reserving "emergency memory"): http://stackoverflow.com/a/27581958/10831

@dominics
Copy link

dominics commented Apr 9, 2017

Attempting to do anything, such as raising the memory limit (which would probably be a bad idea), will use more memory

Just wanted to specifically call this out too (although I totally agree that it could be a bad idea unless the user opts in): raising the memory limit does not use more memory.

  • Calling ini_set the first time in a process uses about 32 bytes. It's unlikely (to my mind, in practical situations) that the user ran out of memory on an attempted allocation of less than this amount, so the call to ini_set will almost always succeed.
  • Calling it a second time in a process doesn't use any memory, and will always succeed, even under OOM conditions.

See, for example, this 3v4l: https://3v4l.org/Xuun2

ini_set('memory_limit', '1M');

register_shutdown_function(function () {
    ini_set('memory_limit', '10M');
    $v = str_repeat(' ', 2 ** 21);
    var_dump(strlen($v));
});

$v = str_repeat(' ', 2 ** 21);

(The first str_repeat causes an OOM error, the second str_repeat allocates the same amount successfully.) So, with that, I think we have all the tools necessary to get this error shipped to Bugsnag successfully, 99% of the time.

@GrahamCampbell
Copy link
Contributor

It is not always to recover, however:

https://3v4l.org/K2Uq4

@GrahamCampbell
Copy link
Contributor

Calling ini_set the first time in a process uses about 32 bytes. It's unlikely (to my mind, in practical situations) that the user ran out of memory on an attempted allocation of less than this amount, so the call to ini_set will almost always succeed.

It's possible people will have disabled that function, or do not want their memory limit messed with.

@dominics
Copy link

dominics commented Apr 9, 2017

@GrahamCampbell I think I've addressed both of those comments.

although I totally agree that it could be a bad idea unless the user opts in

etc.

@GrahamCampbell
Copy link
Contributor

It's not a good idea to test this bug with a recursive stack overflow. That's because xdebug provides a stack limit that on normal PHP instances will hit much earlier (256 recursions by default?) than a memory limit.

Xdebug is generally not installed on production servers, where Bugsnag is primarily used. I'd argue it's a bad practice to install xdebug on production machines, if not for the huge performance impact alone.

@dominics
Copy link

dominics commented Apr 9, 2017

bad practice to install xdebug on production machines

@GrahamCampbell can you take a bit longer and read what I've written? My next sentence was "trying to reproduce in their dev environments"...

@GrahamCampbell
Copy link
Contributor

I think a memory limit being bust by trying to allocate memory for a function call is actually the most common case in the real world, rather than simulated massive strings, since in that case, php's dropping the string from memory, which is why you are able to continue.

@dominics
Copy link

dominics commented Apr 9, 2017

actually the most common case in the real world

I'd probably guess

  • buffered MySQL queries using the mysqlnd driver
  • HTTP response payloads using curl

would both be more common cases than a stack overflow.

@GrahamCampbell
Copy link
Contributor

The most common cases I see are accidental infinite recursion in Eloquent, or the service container. I mean, they are all valid cases, so. We need to keep them all in mind. :)

@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Apr 9, 2017

If we look at the example where we make a large string, PHP really does seem to throw it away (at least, from the evidence I can see - perhaps I'm wrong?), which is why we can continue:

Compare https://3v4l.org/52HjC with https://3v4l.org/MUpBC. See how on the string repeat example, the error is for "1MB", but in the shutdown function, the memory has already been released. This is exactly what is happening in the initial bug report in this issue I think, and then Bugsnag itself is then causing the out of memory exception again, when it tries to report the original.

@dominics
Copy link

dominics commented Apr 10, 2017

PHP really does seem to throw it away (at least, from the evidence I can see - perhaps I'm wrong?), which is why we can continue

Yeah, that's definitely what I've been seeing (and saying: "if the allocation that failed was large, you'll have plenty of available memory to process the error").

Nevertheless, I seem to be having some good success with https://gist.github.com/dominics/61c23f2ded720d039554d889d304afc9 because it copes with OOM errors simulated like e.g.

$arr = [];
while (true) {
    $arr[] = str_repeat(' ', 32);
}

(so, with very low memory available when initially processing the error.) The OOM-on-function-call case remains intractable though - it looks like it errors when invoking the shutdown function (thus the "Unknown" stack trace), so you're right there's nothing to be done there.

Maybe it's just a matter of documenting this limitation, and providing an example of how to catch the OOM errors that can be caught. Finding cases where my ORM was OOMing on huge result sets this way has been pretty helpful.

@GrahamCampbell
Copy link
Contributor

Yes, thanks for the input. I think that while there are limitations here, as well as documenting them, we could also potential recovery from these errors (off by default). I'll put together a PR soon that you can all check out. :)

@aleemb
Copy link
Author

aleemb commented Apr 10, 2017

Just for the record the issue for me personally was not the out of memory exception itself but that turning off BugSnag allowed me to capture it and turning it back on left me in the dark.

The most common cases I see are accidental infinite recursion in Eloquent, or the service container. I mean, they are all valid cases, so. We need to keep them all in mind. :)

Since this is an out of memory exception with or without BugSnag it's a nice-to-have but if it's not technically feasible, then I guess it isn't. On development environments, I wouldn't mind if XDebug helped this along but am not fussed either way.

buffered MySQL queries using the mysqlnd driver
HTTP response payloads using curl

I am experiencing something along these lines and this is the scenario I am hoping gets handled more gracefully. The exception is dumped just fine in Laravel/PHP but enabling BugSnag causes havoc.

I suspect it's not memory usage (memory_get_usage()) but peak memory usage (memory_get_peak_usage()) that the culprit. I don't mean to state the obvious here and without having look under BugSnag's hood, it might be possible that BugSnag can use any techniques such as clobbering variables, reducing it's own call stack, simplifying objects, or flushing buffers to reduce it's peak memory usage.

@GrahamCampbell
Copy link
Contributor

Just for the record the issue for me personally was not the out of memory exception itself but that turning off BugSnag allowed me to capture it and turning it back on left me in the dark.

Yeh, this is for the reason that I explained. Turning off bugsnag means the exception is left unhandled, so you can see it, but turning on Bugsnag obscures it, because Bugsnag crashes.

Since this is an out of memory exception with or without BugSnag it's a nice-to-have but if it's not technically feasible, then I guess it isn't.

I think it's worth documenting the limitations of handling out of memory errors in PHP, but also, to add the capability to the Bugsnag library to be able to recover from such errors if at all possible.

I don't mean to state the obvious here and without having look under BugSnag's hood, it might be possible that BugSnag can use any techniques such as clobbering variables, reducing it's own call stack, simplifying objects, or flushing buffers to reduce it's peak memory usage.

Yes, I agree, however, while this may seem like an obvious solution, there's no guarantee this would actually fix the issue. Ironically, attempting to clear memory may actually end up using more memory, since clearing memory in PHP merely involves setting the "refcount" to 0, rather than actually clearing the memory. We have to rely on PHP's garbage collector to actually free the memory.

@nasyrov
Copy link

nasyrov commented Oct 2, 2017

Hi guys,

I've recently tried to enable Bugsnag support with my app and stumble upon this problem, any progress on that? My app is under the high load and increasing a memory limit means decreasing the number of concurrent requests a server can handle...

@Cawllec
Copy link
Contributor

Cawllec commented Jan 11, 2018

Based on the discussion in this thread I'd suggest looking at @dominics fix for now, and we'll schedule a discussion on this to see what we can do.

@CrixuAMG
Copy link

What is the progress on this issue? We're running bugsnag on our testing server and since we don't get the output of the exception we cannot easily determine the cause of any issue for now.

I'll remove the package for now in order to fix the issues present in the application.

Thanks in advance!

@Cawllec
Copy link
Contributor

Cawllec commented Mar 26, 2018

As indicated in the discussion there isn't really a good way of fixing this for us at the moment. I'd advice you to check out @dominics fix above for a way around this issue for now.

@reinink
Copy link

reinink commented Apr 11, 2018

Just another +1 for solving this.

@GrahamCampbell did you ever put together a PR to optionally allow this behaviour?

@snmaynard snmaynard added feature request Request for a new feature and removed feedback labels Oct 26, 2018
@noamgu
Copy link

noamgu commented Aug 7, 2019

Also happened to me :(

@abigailbramble abigailbramble removed the feature request Request for a new feature label Oct 3, 2019
@abigailbramble abigailbramble added the needs discussion Requires internal analysis/discussion label Oct 3, 2019
@adam1010
Copy link

adam1010 commented Aug 11, 2020

I've fixed the problem in Laravel itself, please comment on the PR to help get it merged:
laravel/framework#33883

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires internal analysis/discussion
Projects
None yet
Development

No branches or pull requests