Call gc_collect_cycles() always twice, fixed in PHP 8.1, required for PHP 8.0 and earlier

[mvorisek]

fixes #90

related with php/php-src#5581

[Ocramius]

Is there a meaningful way to test this? Otherwise, mutation testing will just pick it up.

…

On Fri, Jun 11, 2021, 00:15 Michael Voříšek ***@***.***> wrote: fixes #90 <#90> ------------------------------ You can view, comment on, or merge this pull request online at: #124 Commit Summary - Call gc_collect_cycles() always twice, fixed in PHP 8.1 File Changes - *M* src/CollectTestExecutionMemoryFootprints.php <https://github.com/Roave/no-leaks/pull/124/files#diff-fbe1018d426fb7b79d51f001b7ef28b7527362f05553719d70107ab64e4de778> (2) Patch Links: - https://github.com/Roave/no-leaks/pull/124.patch - https://github.com/Roave/no-leaks/pull/124.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#124>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEFRCJTPTWVNDXNU6FTTSE2RXANCNFSM46PPG6LA> .

[mvorisek]

here is a simple script to reproduce:

https://3v4l.org/snnb3

basically any scenario which has a cyclic reference (otherwise GC is not needed) with at least one object with __destructor method

[Ocramius]

Can we factor it into the test suite? 🤔

…

On Fri, Jun 11, 2021, 09:47 Michael Voříšek ***@***.***> wrote: here is a simple script to reproduce: https://3v4l.org/snnb3 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#124 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEDAQHUHVN6WSWTAOG3TSG5RTANCNFSM46PPG6LA> .

[mvorisek]

test added, when the fix (1st commit) is reverted, false positive leak is detected - see 0d71b6c

lock file also removed, https://github.com/Roave/no-leaks/pulls?q=is%3Apr+author%3Aapp%2Fdependabot-preview PRs are completely unusefull as when installed as composer library/dep, lock file is not used

[mvorisek]

please look at https://github.com/Roave/no-leaks/pull/124/checks?check_run_id=2853673697 failing test, it is failing randomly and it seems unrelated with this PR

[mvorisek]

please look at https://github.com/Roave/no-leaks/pull/124/checks?check_run_id=2853673697 failing test, it is failing randomly and it seems unrelated with this PR

did you read it?

[Ocramius]

@mvorisek I see no failures there?

Are we talking different things perhaps?

My initial question around this patch was a lack of testing around the added gc_collect_cycles() call: tests seem to have been added.

After that, a number of unrelated changes that remove composer.lock were introduced, and those need to be reverted (or composer.lock updated to a state that leads to green tests).

[mvorisek]

@mvorisek I see no failures there?

Are we talking different things perhaps?

see https://github.com/Roave/no-leaks/runs/2853673697?check_suite_focus=true for example, testing is unstable, about every 3/4th push did not pass testing, even if another push with unchanged files passes.

[Ocramius]

Is that the previous state, or the current state?

As for why that happens, I am not able to identify a root cause at the moment.

[mvorisek]

Is that the previous state, or the current state?

As for why that happens, I am not able to identify a root cause at the moment.

happend with 1dae150 (sha obtained from https://github.com/Roave/no-leaks/runs/2853673697?check_suite_focus=true#step:2:150 )

I noticed that at least 2 times. I belive with enough pushes it must be reproducible with the latest PR update and probably also with the default repo branch.

[mvorisek]

@Ocramius any more feedback except the discovered but unrelated CI unstability?

[Ocramius]

@Ocramius any more feedback except the discovered but unrelated CI unstability?

The instability is worrisome, but unsure how to tackle it.

Overall, I think the patch is fine (see just the last comments)

[mvorisek]

feedback fixed and changes squashed to one commit

[Ocramius]

Thanks! Cutting a release now :-)

[mvorisek]

Thank too, just a small discovery:

something is wrong with escaping in the rel. notes:

[Ocramius]

Yep, that's known - it's potentially an issue with https://github.com/jwage/changelog-generator