8000 Allow suspending fibers in destructors · Issue #11389 · php/php-src · GitHub
[go: up one dir, main page]
Skip to content

Allow suspending fibers in destructors #11389

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.

Sign up for GitHub

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

Jump to bottom
Closed
nicolas-grekas opened this issue Jun 7, 2023 · 6 comments
Closed

Allow suspending fibers in destructors #11389

nicolas-grekas opened this issue Jun 7, 2023 · 6 comments
Labels
Category: Fibers Category: GC Feature Status: Needs Triage

Comments

@nicolas-grekas
Copy link
Contributor

Description

Right now, when trying to suspend the current fiber in a destructor, the engine throws "Cannot switch fibers in current execution context".

This has been discussed a bit with @trowski and @kelunik in nicolas-grekas/symfony#43 (review)

From that discussion, the reason is that the garbage collector would need to be made safe for re-entry from multiple fibers.

Not being able to do i/o (which is what fibers are most useful for) in destructors kinda reintroduces the “What color is your function?” problem, which fibers were designed to avoid.

I totally get that this might be challenging, but this limitation should be removed if possible. 🙏
@arnaud-lb
Copy link
Member

Destructors/finalizers exist for the kind of use-cases that may require I/O (e.g. closing a network connection or flushing a buffer). With an I/O framework that uses Fibers, this may necessitate Fiber switching, so providing support for this would be beneficial.

One solution would be to execute the destructors after collection. This seems to be common practice because this solves a number of issues.

However, this raises the following question: Does the Fiber in which the destructors are called matter? I'm not very familiar with Amp internals: does it matter if the caller may be any Fiber, including the scheduler Fiber (if there is one) or the main "Fiber"?

An other issue, similar to the one raised by @trowski in nicolas-grekas/symfony#43 (comment), is that if a destructor suspends and the Fiber is never resumed, the remaining destructors may never be called. A solution to this might be to execute the remaining destructors after switching.

@mvorisek
Copy link
Contributor
mvorisek commented Jun 23, 2023 •
edited
Loading

if a destructor suspends and the Fiber is never resumed, the remaining destructors may never be called. A solution to this might be to execute the remaining destructors after switching.

This is already the current behaviour - https://3v4l.org/6AvfW - finally blocks and destructors are executed in the same fiber even if suspended.

@arnaud-lb
Copy link
Member
arnaud-lb commented Jun 24, 2023 •
edited
Loading

Yes, currently the destructors are called in the Fiber that happened to be active when the GC was triggered. What I meant is that if the GC queues 2 destructors, and one suspends, then the other one may never be executed. So I was suggesting that after suspend, we execute the remaining destructors in the new active Fiber.

class A {
    public function __destruct() { Fiber::suspend(); }
}
class B {
    public function __destruct() { }
}

$x = new stdClass;
$x->x = $x;
$x->a = new A;
$x->b = new B;
$x = null;
gc_collect_cycles(); // Would queue $a->__destruct() and $b->__destruct()

In the example above, when $a->__destruct() suspends, $b->__destruct() would be executed in the fiber we switched to after suspending.

@arnaud-lb
Copy link
Member
arnaud-lb commented Jun 24, 2023 •
edited
Loading

This made me realize that fiber switching is not only disallowed during GC, but also when destroying objects outside of GC.

@trowski @kooldev do you remember why you had to disable switching in fdc22744a89 ?

@trowski
Copy link
Member
trowski commented Jul 9, 2023

does it matter if the caller may be any Fiber, including the scheduler Fiber (if there is one) or the main "Fiber"?

Generally it does not matter which fiber calls the destructor. We wouldn't want a suspending destructor to be invoked within the event loop (scheduler) fiber, but we wrote the event loop to execute callbacks and destroy the reference to that callback within another fiber separate from the event loop fiber.

@trowski @kooldev do you remember why you had to disable switching in fdc22744a89 ?

We disabled switching fibers in all contexts for consistency. It would be surprising to be able to switch fibers in a destructor, unless the destructor was being run as part of cyclic garbage collection.

To make fiber switching in destructors safe, it appears we do not need to make the garbage collector itself safe for re-entry if we collect objects to be destructed, then execute destructors after collection. That way no user code can be executed during cyclic collection. We then only need to ensure that a destructor suspending during execution does not block the destructor of other objects collected during garbage collection.

To accomplish this, I think we can use a strategy similar to that which we used in the Revolt event loop to execute loop callbacks, which also may suspend during execution and should not block other loop callbacks from being immediately executed. After garbage collection collects the objects to be destroyed, we switch fibers before executing the queued destructors. If the fiber is not suspended within a destructor, we can execute the next destructor on the same fiber. If that fiber does suspend in the destructor, a new fiber is allocated to execute the next destructor, replacing the fiber which was suspended.

A fiber suspended during destructor execution will live on in use space, but once resumed, returning from the destructor, the fiber will be destroyed because no further references will exist in either user space or internally.

This strategy would require a global to store the fiber for executing destructors and at least one fiber switch before and after executing destructors. Fiber switches are very fast and would only occur after the GC is run, not on every destructor call. The fiber for executing destructors would only need to be created when cycles are collected by the GC.

@arnaud-lb I believe you might have been alluding to this strategy in your post above. Is this something you'd have time to try implementing?

@arnaud-lb
Copy link
Member

@arnaud-lb I believe you might have been alluding to this strategy in your post above. Is this something you'd have time to try implementing?

I was indeed alluding to a similar strategy. You have gone a bit further though, by running the destructors in a separate fiber so that a suspending destructor will not disrupt the application's fibers.

This should probably target 8.4. There are a few things I want to do before working on this, but I should be able to implement it before 8.4 (unless you or someone else want to, of course).

@nicolas-grekas nicolas-grekas mentioned this issue Oct 12, 2023
Closed
@arnaud-lb arnaud-lb mentioned this issue Feb 21, 2024
Closed
@nicolas-grekas nicolas-grekas mentioned this issue Mar 6, 2024
Merged
fabpot added a commit to symfony/symfony that referenced this issue Aug 19, 2024
@fabpot
feature #54179 [HttpClient] Add support for amphp/http-client v5 (nic…
a44311e 
…olas-grekas)

This PR was merged into the 7.2 branch.

Discussion
----------

[HttpClient] Add support for amphp/http-client v5

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | #52008
| License       | MIT

This PR adds support for amphp/http-client version 5 as a transport for our HttpClient component.

It started as a draft at nicolas-grekas#43, which helped spot that PHP is missing the capability to suspend fibers in destructors. This was reported as php/php-src#11389 and is being fixed at php/php-src#13460.

Since the fix for php-src is going to land on PHP 8.4, using amphp/http-client version 5 will require php >= 8.4.

The implementation duplicates the one we have for v4 with the needed changes to use the v5 API. The difference are not big in size of code, but they're very low level (generators vs fibers). That would be quite useless to factor IMHO.

Commits
-------

a5fb61c [HttpClient] Add support for amphp/http-client v5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Category: Fibers Category: GC Feature Status: Needs Triage
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nicolas-grekas @arnaud-lb @trowski @mvorisek
0