Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

frankenphp: fix loop increment and break behavior #173

Closed
wants to merge 1 commit into from

Conversation

tamcy
Copy link
Contributor

@tamcy tamcy commented Jun 13, 2024

The current Runtime doesn't behavior as documented.

        } while ($ret && (-1 === $this->loopMax || ++$loops <= $this->loopMax));

Found two issues in my testing, hence this PR:

  1. when $ret is true (which seems to be always the case?), the remaining conditions won't be evaluated, so $loops will never increment.
  2. Setting $loopMax to 1 will result in 2 requests be handled before resetting.

I'm not very sure of what $ret does (I thought this will return false when an unrecoverable error occurs, turns out it won't), but in anyway it dosen't sound right for the number of request not incrementing when the response is a successful one.

@alexander-schranz
Copy link
Member

@dunglas can you have a look at this.

Copy link
Contributor

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Good catch.

@dunglas
Copy link
Contributor

dunglas commented Jun 13, 2024

$ret is used to terminate gracefully the worker script when FrankenPHP is stopped.

@tamcy
Copy link
Contributor Author

tamcy commented Jun 14, 2024

After a night I reread my points and found point 1 to be ridiculous - the interpreter should evaluate the second condition because it's an AND!

Here's the story: yesterday, I spent more than an hour trying to figure out why the runtime won't reset itself even when FRANKENPHP_LOOP_MAX is set to 1 when worker number is 1. I found that the $loop variable doesn't increase at all in the original version by placing logs here and there, also after the do-while loop, and also found that the loop doesn't exist at all. Not until I move the loop increment from while to the loop. I wasn't sure why I jumped to this absurd conclusion, perhaps I got irrational due to the behavior.

(The following codes work as expected.)

<?php
class Foo
{
    private $loopMax = 10;
    public function run()
    {
        $loops = 0;
        do {
            echo "loop $loops\n";
            $ret = true;

            gc_collect_cycles();

        } while ($ret && (-1 === $this->loopMax || ++$loops < $this->loopMax));
    }
}


$foo = new Foo();
$foo->run();

I request to put this PR on hold. The second fix is still correct; I'd like to fix my commit message first.

Sorry for the inconvenience caused.

@tamcy
Copy link
Contributor Author

tamcy commented Jun 14, 2024

The loop increment is indeed working fine. I've submitted #174 as a replacement. Sorry for the false alarm!

alexander-schranz pushed a commit that referenced this pull request Jun 14, 2024
Fixes the issue that the loop doesn't exist when `$loopMax` is reached,
but `$loopMax+1`. Please refer to
#173 for details.
Nyholm pushed a commit to php-runtime/frankenphp-symfony that referenced this pull request Jun 14, 2024
Fixes the issue that the loop doesn't exist when `$loopMax` is reached,
but `$loopMax+1`. Please refer to
php-runtime/runtime#173 for details.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants