tecnickcom / TCPDF

Official clone of PHP library to generate PDF documents and barcodes
https://tcpdf.org
Other
4.18k stars 1.51k forks source link

Fix garbage collection #509

Closed deguif closed 1 year ago

deguif commented 2 years ago

When registering a shutdown function referencing to the tcpdf instance, it prevents this instance to be garbage collected. The desctructor call destroy(true) so the destruction result should be the same, but the object will be destroyed once it is not referenced anymore.

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

deguif commented 2 years ago

By investigating a long running script that generates hundred of PDFs (with memory increasing). When debugging inside the __destruct method, I was surprised to see it was never called, until the script ended (even when gc_collect_cycles was called after each pdf generation).

williamdes commented 2 years ago

By investigating a long running script that generates hundred of PDFs (with memory increasing). When debugging inside the __destruct method, I was surprised to see it was never called, until the script ended (even when gc_collect_cycles was called after each pdf generation).

Merci ! Could this be proved by some code written in a file like https://github.com/tecnickcom/TCPDF/blob/main/examples/example_066.php ?

nicolaasuni commented 2 years ago

The _destroy function also deletes temporary files. Removing this will leave zombie files?

deguif commented 2 years ago

@nicolaasuni, that should not be the case, as the _destroy method is called on the destructor. That way it will be destroyed natively by the PHP engine, when object instance got out of scope (not referenced anymore).

The issue here is that registering a shutdown function on the constructor PHP keep a reference of this object, which prevents the destructor to be called by the PHP engine when object goes out scope.

deguif commented 2 years ago

Just rebased the branch

deguif commented 1 year ago

Any way to get this one merged?

williamdes commented 1 year ago

@nicolaasuni does reviews from time to time, months apart and randomly. So it's up to us to wait once a happy day arrives :)

nicolaasuni commented 1 year ago

In order to be able to merge this the license/cla must be signed.

codecov-commenter commented 1 year ago

Codecov Report

Merging #509 (c1cc9cc) into main (ca70b94) will increase coverage by 0.12%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #509      +/-   ##
==========================================
+ Coverage   72.80%   72.92%   +0.12%     
==========================================
  Files         127      127              
  Lines       21606    21608       +2     
==========================================
+ Hits        15730    15758      +28     
+ Misses       5876     5850      -26     
Flag Coverage Δ
php-7.1-ubuntu-latest 72.92% <ø> (?)
php-7.2-ubuntu-latest 72.80% <ø> (-0.01%) :arrow_down:
php-7.3-ubuntu-latest 72.79% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tcpdf.php 71.65% <ø> (+0.16%) :arrow_up:
tcpdf_barcodes_1d.php 87.24% <0.00%> (+0.02%) :arrow_up:
include/tcpdf_static.php 52.97% <0.00%> (+0.20%) :arrow_up:
include/tcpdf_fonts.php 43.89% <0.00%> (+0.26%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.