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

Event loop hangs after calling exists #75

Closed
razshare opened this issue Jun 17, 2023 · 6 comments
Closed

Event loop hangs after calling exists #75

razshare opened this issue Jun 17, 2023 · 6 comments
Labels

Comments

@razshare
Copy link

razshare commented Jun 17, 2023

Hey, I'm trying to prepare migrating a large codebase from amp v2 to v3 and so far everything has been really smooth, which is impressive, also the documentation and the short descriptions on the github releases page saying what replaced what from v2 to v3 helped a lot.

However I'm having an issue with \Amp\File\exists, it seems to be causing revolt's event loop to hang.

I prepared a repository that reproduces the error as I'm seeing it
https://github.com/tncrazvan/amp-v3-event-loop-hangs-after-exists

I'm running the script and then my event loop seems to hang forever.
image

However if I run some other async operation other than exists, everything works fine, the event loop stops as expected.

These are my php version and modules

PHP 8.2.7 (cli) (built: Jun  8 2023 15:27:40) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.7, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.7, Copyright (c), by Zend Technologies
    with Xdebug v3.2.1, Copyright (c) 2002-2023, by Derick Rethans
[PHP Modules]
calendar
Core
ctype
curl
date
dom
exif
FFI
fileinfo
filter
ftp
gettext
hash
iconv
json
libxml
mbstring
mysqli
mysqlnd
openssl
pcntl
pcre
PDO
pdo_mysql
Phar
posix
random
readline
Reflection
session
shmop
SimpleXML
sockets
sodium
SPL
standard
sysvmsg
sysvsem
sysvshm
tokenizer
uuid
xdebug
xml
xmlreader
xmlwriter
xsl
yaml
Zend OPcache
zlib

[Zend Modules]
Xdebug
Zend OPcache

And this is my os release

Distributor ID:	Pop
Description:	Pop!_OS 22.04 LTS
Release:	22.04
Codename:	jammy
@kelunik kelunik added the bug label Jul 23, 2023
@kelunik
Copy link
Member

kelunik commented Jul 23, 2023

Thanks for reporting! I can reproduce the issue, seems to be the ipc or result pipe of amphp/parallel.

@trowski
Copy link
Member

trowski commented Dec 27, 2023

@tncrazvan Was this fixed with amphp/parallel@v2.2.5?

@trowski
Copy link
Member

trowski commented Dec 27, 2023

Pulled your reproducer and it no longer hangs.

@trowski trowski closed this as completed Dec 27, 2023
@razshare
Copy link
Author

razshare commented Jan 6, 2024

Hey, I've just tested the reproducer again and the issue seems to persist.

I've managed to debug it a bit.
From what I've gathered so far this might be a policy issue on how workers are executed rather than a bug.

I've noticed that amphp/parallel will start at least 1 worker (up to 32) whenever submitting any task, aka populating the pool.
I believe this to be the relevant piece of code that starts the workers.

These pool workers seem to be staying up forever, which is fair enough, that's what a pool worker is supposed to do.

Now the issue I think is that there's currently no way to hint to amphp/parallel that my program is about to end and I want to destroy my workers (unless there actually is a way to do that and I'm missing it).

I've come to this conclusion by checking my system processes and debugging the revolt event loop.

I've thrown a few echos into the revolt loop just to illustrate what's going on (I'm altering this part to be clear)

private function createLoopFiber(): void
{
    $this->fiber = new \Fiber(function (): void {
        $this->stopped = false;

        // Invoke microtasks if we have some
        $this->invokeCallbacks();

        /** @psalm-suppress RedundantCondition $this->stopped may be changed by $this->invokeCallbacks(). */
        while (!$this->stopped) {
            echo "looping\n";       // <======= HERE
            if ($this->interrupt) {
                $this->invokeInterrupt();
            }

            if ($this->isEmpty()) {
                return;
            }

            $previousIdle = $this->idle;
            $this->idle = true;

            $this->tick($previousIdle);
            $this->invokeCallbacks();
        }
    });
}

And this is the result

image

The program should and after the it exists echo, but as you can see it keeps going, because the workers are still up.

Note
The event loop also seems to be alternating between blocking and non blocking dispatching here, it happens because this variable alternates between true and false forever, although I'm not sure if that's supposed to be happening or not.
I'm not well versed in the internals of the project so I thought I should point that out, maybe it's helpful.

And finally I can see the worker among my system processes
image

As soon as I kill that process, the event loop unlocks and the program ends.


I personally don't think the workers are behaving wrong... I'm ok with them going on forever, but is there a way to stop them safely and on demand?

trowski added a commit to amphp/parallel that referenced this issue Jan 7, 2024
@trowski
Copy link
Member

trowski commented Jan 7, 2024

Thanks @tncrazvan for looking into this again and finding it was still an issue. I seem to have forgotten I had UV enabled when I tested, so of course a parallel worker wasn't being used for file access.

I found where there was still a referenced event-loop callback waiting for the worker result and refactored to avoid that callback being referenced unless the result is actively being awaited. This should now be fixed with amphp/parellel@v2.2.6.

renovate bot added a commit to ben-challis/sql-migrations that referenced this issue Jan 7, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [amphp/parallel](https://github.com/amphp/parallel) | `2.2.5` ->
`2.2.6` |
[![age](https://developer.mend.io/api/mc/badges/age/packagist/amphp%2fparallel/2.2.6?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/amphp%2fparallel/2.2.6?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/amphp%2fparallel/2.2.5/2.2.6?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/amphp%2fparallel/2.2.5/2.2.6?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>amphp/parallel (amphp/parallel)</summary>

### [`v2.2.6`](https://github.com/amphp/parallel/releases/tag/v2.2.6):
2.2.6

[Compare
Source](https://github.com/amphp/parallel/compare/v2.2.5...v2.2.6)

#### What's Changed

- Fixed hang during shutdown if a process or thread context has not
ended (fixes
[amphp/file#75](https://github.com/amphp/file/issues/75)).

**Full Changelog**:
amphp/parallel@v2.2.5...v2.2.6

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/ben-challis/sql-migrations).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEyMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@razshare
Copy link
Author

razshare commented Jan 7, 2024

@trowski I can confirm from my side everything works smooth as butter now! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants