The Wayback Machine - https://web.archive.org/web/20221004035630/https://github.com/symfony/symfony/pull/43678
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

Jump to bottom

[Process] Add support for Fiber #43678

Closed
wants to merge 1 commit into from
Closed

[Process] Add support for Fiber #43678

wants to merge 1 commit into from

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Oct 23, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

Example:

(Note: The loop is very naive, but it's for demo purpose)

<?php

use Symfony\Component\Process\Process;

require __DIR__ . '/vendor/autoload.php';

function logg(string $message, string ...$params)
{
    dump(sprintf($message, ...$params));
}

class Loop
{
    public function __construct(
        private array $fibers
    ) {
    }

    public function run()
    {
        foreach ($this->fibers as $k => $fiber) {
            logg('Fiber %d started', $k);
            $fiber->start();
        }

        while ($this->fibers) {
            foreach ($this->fibers as $k => $fiber) {
                if ($fiber->isTerminated()) {
                    logg('Fiber %d finished', $k);
                    unset($this->fibers[$k]);
                    continue;
                }

                $fiber->resume();
            }

            usleep(100_000);
        }
    }
}

// // Sync
// $process = new Process(['sleep', 1]);
// $process->mustRun();

// Async
$fiber1 = new Fiber(function () {
    $process = new Process(['sleep', 1]);
    $process->mustRun();
});
$fiber2 = new Fiber(function () {
    $process = new Process(['sleep', 2]);
    $process->mustRun();
});
$fiber3 = new Fiber(function () {
    $process = new Process(['sleep', 3]);
    $process->mustRun();
});

// dump($fiber1);die();


$loop = new Loop([$fiber3, $fiber2, $fiber1]);
$loop->run();
process-fiber.mp4

@lyrixx lyrixx added the Process label Oct 23, 2021
@carsonbot carsonbot added this to the 5.4 milestone Oct 23, 2021
@nicolas-grekas

This comment has been minimized.

Sign in to view
@lyrixx

This comment has been minimized.

Sign in to view
@b-viguier
Copy link

b-viguier commented Oct 23, 2021

Glad to see that you consider using Fibers! 🤗
Just some thoughts about this:

  • What if the fiber is never resumed?
  • What if the fiber is resumed with throw?
  • What if the fiber is resumed without calling usleep in the meantime? (To be honest, I'm not sure Process actually needs this…)

My point is that it could be interesting to define a contract about "how to use safely a SF Process in a Fiber".
A part of this contract could also be transmitted via the suspend function, a kind of promise to be resumed in n microseconds, or when a goal is reached… 🤷

I have no particular idea about this, I'm not sure if Symfony needs a complete async tools set, but I'm just wondering if this suspend can be an open door to wrong usages.

Anyway, this PR has my full attention 😉👍

@lyrixx
Copy link
Member Author

lyrixx commented Oct 23, 2021 •
edited

Glad to see that you consider using Fibers!

Thanks a lot for your feedback

  • What if the fiber is never resumed?

The process will last, but it's exactly the same as when you write the follow code

$p = new Process['sleep', '10'];
$p->start();
// Nothing here

So IMHO, nothing has to be done here

  • What if the fiber is resumed with throw?

What would be the use case to resume it with throw?

  • What if the fiber is resumed without calling usleep in the meantime? (To be honest, I'm not sure Process actually needs this…)

Indeed, I was not sure about the use case, but actually, it eases the async handling of process.

My point is that it could be interesting to define a contract about "how to use safely a SF Process in a Fiber".

I totally agree with that 👍🏼, and more generally in Symfony

I'm just wondering if this suspend can be an open door to wrong usages.

Yes, it can. I said that, because we keeps reading issue where people do really strange things :) I guess it's how OSS works :)

@b-viguier
Copy link

b-viguier commented Oct 23, 2021

The process will last, but it's exactly the same as when you write the follow code [...]
So IMHO, nothing has to be done here

👍

What would be the use case to resume it with throw?

Since it's possible, we can expect that someone will try 😅.
But it could a way to stop a too long process (timeout), or to cancel it for other external reasons (too much opened fibers, too much memory... 🤷‍♂️)

Yes, it can. I said that, because we keeps reading issue where people do really strange things :) I guess it's how OSS works :)

😅😉

@fabpot fabpot modified the milestones: 5.4, 6.1 Oct 29, 2021
@fabpot
Copy link
Member

fabpot commented Mar 26, 2022

What's the status here? @lyrixx?

@lyrixx
Copy link
Member Author

lyrixx commented Mar 28, 2022 •
edited

I think there is consensus yet on how to add fibrer support in Symfony. Let's close it

But if someone want it, we could also merge it.

@lyrixx lyrixx closed this Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelwurtz joelwurtz

@nicolas-grekas nicolas-grekas

@stof stof

@OskarStark OskarStark

@derrabus derrabus

Assignees
No one assigned
Labels
Feature Process Status: Needs Review
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@lyrixx @nicolas-grekas @b-viguier @fabpot @joelwurtz @stof @OskarStark @derrabus @carsonbot