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

fix Coroutine\Barrier mem leak #94

Merged
merged 2 commits into from
Feb 10, 2021
Merged

Conversation

Appla
Copy link
Contributor

@Appla Appla commented Jan 28, 2021

the static property $cancel_list hold the value set in __destruct in some case.
after barrier object destructed by set it's reference by null, we can unset this that value added in __destruct

the following test case could trigger this issue

    /**
     * Test without execution switching between coroutines.
     *
     * @covers \Swoole\Coroutine\Barrier
     */
    public function testNoCoroutineSwitching()
    {
        run(function () {
            $barrier = Barrier::make();
            $count = 0;
            $N = 4;
            foreach (range(1, $N) as $i) {
                \Swoole\Coroutine::create(function () use ($barrier, &$count) {
                    $count++;
                });
            }
            Barrier::wait($barrier);

            $this->assertSame($N, $count, 'The parent coroutine keeps running without switching execution to child coroutines.');
        });
    }

@deminy
Copy link
Member

deminy commented Jan 28, 2021

Linking to a similar PR (#69) submitted by @sy-records .

Copy link
Member

@deminy deminy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. My only suggestion is that if there are multiple improvements/bug fixes to address, please consider separating them into different PRs, and each focuses on a specific issue.

@matyhtf matyhtf merged commit bd196e9 into swoole:master Feb 10, 2021
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.

5 participants