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.4] Attempt to fix container bug #18812

Merged
merged 3 commits into from
Apr 16, 2017
Merged

[5.4] Attempt to fix container bug #18812

merged 3 commits into from
Apr 16, 2017

Conversation

laurencei
Copy link
Contributor

@laurencei laurencei commented Apr 15, 2017

This is an attempt to fix the bug introduced in 5.4.16 as described in #18806

From my testing - it looks when resolving a singleton, the collection never pops the $this->make[] array - resulting in additional new arrays being constantly added to the existing array every time a call to resolve() occurs. Because these new arrays are never released when resolving a singleton - it results in the run away memory usage.

This is particularly noticeable in an Eloquent object, where calls that include date attribute casting can easily result in thousands of calls to resolve() - which keeps adding arrays to $this->make[], and never pops them off when finished.

My testing shows memory returns to the same as 5.4.15 with this PR.

My major concern; is there another effect on the container this PR might have that I'm not considering? Especially for the makeWith() functions?

I ran through the container code - and I dont believe $this->with is ever touched until after the container confirms it is not resolving something that is already resolved as a singleton. So by moving it down until after the singleton test, we remove the increased memory usage...

@rimace
Copy link
Contributor

rimace commented Apr 15, 2017

This fixed the increased memory usage bug, thanks.

@taylorotwell taylorotwell merged commit ebe568c into laravel:5.4 Apr 16, 2017
@taylorotwell
Copy link
Member

We have tests for makeWith so if they pass it's probably fine.

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.

3 participants