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

Cycle detection doesn't work well with fibers #1752

Closed
kelunik opened this issue Aug 22, 2022 · 2 comments
Closed

Cycle detection doesn't work well with fibers #1752

kelunik opened this issue Aug 22, 2022 · 2 comments
Labels
Milestone

Comments

@kelunik
Copy link
Contributor

kelunik commented Aug 22, 2022

Cycle detection is broken when using fibers, because it assumes that if the function is entered multiple times, it's a cycle. With fibers, the function might be entered multiple times in different fibers if one of them suspends somewhere within the function.

What are your thoughts to moving cycle detection to an interface? There could be a NullCycleDetector that doesn't detect cycles at all, a CountingCycleDetector and then a FiberLocal based implementation that keeps track of the depth for each fiber separately. I'm of course also fine with fibers being supported natively, but I don't think it's a good idea to put this logic into Logger itself in that case.

@Seldaek
Copy link
Owner

Seldaek commented Aug 23, 2022

Given that concurrency isn't really the default model for PHP still, for now you'd need to call $logger->useLoggingLoopDetection(false) (which was added for Swoole "support" 9c1566a) - I'm not sure how the FiberLocal impl would look like, as I am not super familiar with fibers tbh. But I'm open to supporting that if possible as I am sure fibers will only see more usage with time.

@Seldaek Seldaek added this to the 2.x milestone Aug 23, 2022
@kelunik
Copy link
Contributor Author

kelunik commented Aug 23, 2022

Fibers each have their own call stack. Fiber locals are basically variables bound to a specific fiber, so a bit like static, but per fiber. I can work on a PR. I guess FiberLocal doesn't have to be used directly, as clearing isn't required here, so support can be built-in.

kelunik added a commit to kelunik/monolog that referenced this issue Aug 23, 2022
We keep a separate depth count per fiber.

Fixes Seldaek#1752.
Seldaek pushed a commit that referenced this issue Feb 4, 2023
* Fix cycle detection within fibers

We keep a separate depth count per fiber.

Fixes #1752.

* Avoid additional call to Fiber::getCurrent()

Suppresses phpstan errors, as they're false positives.
@Seldaek Seldaek added Bug and removed Feature labels Feb 4, 2023
@Seldaek Seldaek closed this as completed Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants