Skip to content

Conversation

kooldev
Copy link
Contributor

@kooldev kooldev commented Jun 11, 2021

This fixes a memory leak that occurs when symmetric coroutines ares used like this:

await([
	async(fn() => 1),
	async(fn() => 2)
]);

In this case control will be transfered from main to fiber 1 to fiber 2 and back to main. Without this PR this code will result in a memory leak (fiber 1 cannot be released). The fiber context of fiber 1 has to be destroyed immediately after switching to fiber 2 because it is already marked as dead and will never be resumed. The context of fiber 2 is destroyed when main is resumed. Adding additional fibers in the example will fail to destroy every fiber context except for the last one.

@trowski I discovered this bug due to zend_fiber_stack being a tracked allocation after your last commit. 😉

@trowski
Copy link
Member

trowski commented Jun 12, 2021

Good find, that's a subtle bug (that is, before implementing a symmetric coroutine 😆). I'll add a test for it in zend_test.

@trowski trowski merged commit 039f0fe into php:master Jun 12, 2021
@kooldev kooldev deleted the fiber-destroy-context branch June 12, 2021 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants