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

Destruct image to cleanup memory #682

Merged
merged 1 commit into from
Dec 27, 2015
Merged

Destruct image to cleanup memory #682

merged 1 commit into from
Dec 27, 2015

Conversation

cmodijk
Copy link
Contributor

@cmodijk cmodijk commented Dec 21, 2015

In #633 we where encountered a lot of memory leak using the imagick extension. Imagick and Imagine fix that by using the __destruct method but because we use this in a long running process the __destruct is never getting called. This pull requests fixes that issues.

@makasim
Copy link
Collaborator

makasim commented Dec 21, 2015

I dont like it, kind of hack. We can add an event dispatch there, like POST_FILTER or so and you can add a listener with custom logic in your app.

@cmodijk
Copy link
Contributor Author

cmodijk commented Dec 21, 2015

@makasim I don't like it to but it don't know if an event is going to help because in the event you don't know of the imagick object is different than the previous one. And if you __destruct the images that is still being used you get other problems and errors from that.

@makasim
Copy link
Collaborator

makasim commented Dec 21, 2015

Okay, can you add a comment there (with a link to the issue) cuz it is not obvious from the code what it solves.

@makasim
Copy link
Collaborator

makasim commented Dec 26, 2015

@cmodijk ping

@cmodijk
Copy link
Contributor Author

cmodijk commented Dec 27, 2015

@makasim Sorry! To what issue do you want to point the link? This pull request or the original problem?

@cmodijk
Copy link
Contributor Author

cmodijk commented Dec 27, 2015

@makasim I added a comment but I'm not so good with words in English could you please check the comments?

makasim added a commit that referenced this pull request Dec 27, 2015
Destruct image to cleanup memory
@makasim makasim merged commit de17b0a into liip:master Dec 27, 2015
@mattjanssen
Copy link

Thank you! I spent hours yesterday debugging this typical-PHP memory leak. I was about to submit a PR, but I see you took care of it. 👍

I also used the __destruct() method, even though it feels gross; I could think of no better way. The comments are valuable.

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