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

Allow Bugsnag\Handler to handle OOMs #621

Merged
merged 5 commits into from
Feb 2, 2021
Merged

Allow Bugsnag\Handler to handle OOMs #621

merged 5 commits into from
Feb 2, 2021

Conversation

imjoehaines
Copy link
Contributor

Goal

Currently we are able to report OOMs that are caused by a single, large allocation as PHP doesn't actually allocate the memory so there's plenty left for us to use. However, when an OOM happens due to lots of small allocations, we crash as there's not enough memory left to send the report

Design

Bugsnag\Handler will now increase the memory limit on an OOM by the number of bytes specified in the new memoryLimitIncrease option (5MiB default)

To do this successfully, we need to reserve a bit of memory so that we can:

  1. check if an error happened using error_get_last
  2. check if it was an OOM

We don't reserve all of the memory that we need upfront, as this would be a big overhead on every request when it shouldn't be necessary most of the time. 32K was chosen because error_get_last can cause PHP to re-allocate chunks of memory that it has already allocated. This can lead to a second OOM if the chunks it attempts to re-allocate are larger than the available memory. 32K seems to be a happy medium between being small enough to not matter and large enough to allow error_get_last to be called, but this can be tweaked in future

Changeset

  • Added a new configuration option memoryLimitIncrease (5MiB default), with getters & setters in Client & Configuration
  • Handler reserves 32K of memory when the shutdown function is registered
  • Handler increases the current memory limit by memoryLimitIncrease if an OOM is detected

Testing

New PHPT tests cause OOMs and check delivery

We only grab a small amount as we don't want to reserve 5 MiB for
every single request
This will control the amount of memory to claim when an OOM happens
@imjoehaines imjoehaines marked this pull request as ready for review January 28, 2021 09:53
@imjoehaines imjoehaines requested a review from kattrali January 28, 2021 09:53
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

nice work!

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

Successfully merging this pull request may close these issues.

2 participants