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

Global queue #28

Merged
merged 16 commits into from
May 3, 2016
Merged

Global queue #28

merged 16 commits into from
May 3, 2016

Conversation

jsor
Copy link
Member

@jsor jsor commented May 5, 2015

This PR introduces a global task queue to eliminate the problem of deep recursion as discussed in #22.

As noted in this issue, this also opens the possibility to implement future-turn resolutions (#4) through a event loop based queue implementation (example: https://gist.github.com/jsor/52bde3f82014e3898758 for a React\EventLoop based implementation).

Closes #22

@WyriHaximus
Copy link
Member

Shiny! I'll run some tests against it tonight 👍

@jsor
Copy link
Member Author

jsor commented May 5, 2015

Great! I've already successfully run the test suites of react/filesystem, react/react, guzzle and recoil against this branch. But more tests are appreciated!

@WyriHaximus
Copy link
Member

Awesome! Going to run test against a specific function's early dev version of https://github.com/WyriHaximus/TickingPromise that was causing segfaults

@WyriHaximus
Copy link
Member

Right just had the time to run some tests against it. First off the initial version of WyriHaximus\React\tickingFuturePromise still crashes which is not a very big surprise as that chains promises which ends up very unnatural for PHP once you chain a couple of thousand.

The other thing I've noticed that running https://github.com/WyriHaximus/TickingPromise/blob/master/examples/ticking_future.php on the latest tag runs for +/- 24 - 28 seconds while with this PR it runs at 30 seconds solid. Not sure if that is my laptop messing things up maybe you can give it a swirl? Also not that that file is doing 307.200 checks before resolving the promise 😉 .

@jsor
Copy link
Member Author

jsor commented May 5, 2015

Not sure what you're seeing as the "problem" here. That it's slower with the queue (yes, it is slightly slower) or that the time is not longer varying?

@WyriHaximus
Copy link
Member

WyriHaximus commented May 5, 2015 via email

@jsor
Copy link
Member Author

jsor commented Sep 7, 2015

ping @cboden @clue @WyriHaximus

@WyriHaximus
Copy link
Member

LGTM 👍

This reverts commit cc6f1c3.

Note: The task must be kept on the queue until after it is called.
Otherwise drain() will be called recursively.
@cboden
Copy link
Member

cboden commented Oct 8, 2015

Looks good to me. I'm not sure if my issue belongs here but where would ReactLoopQueue live? Adding it here would add a new dependency and adding it in React/EventLoop would do the same. 🐔 🥚

@WyriHaximus
Copy link
Member

@cboden react/promise-loop-queue?

@cboden
Copy link
Member

cboden commented Oct 8, 2015

What about PromiseTimer...sort of fits the bill...at least requires both as deps.

@WyriHaximus
Copy link
Member

Could work, but quickly judging by that package it is focused on promises and timers, not promises queues. Would look out of place imho.

@jsor
Copy link
Member Author

jsor commented Oct 8, 2015

I don't think it fits into reactphp/promise-timer. If anything, this could become a standalone package as @WyriHaximus suggested.

@jsor jsor added this to the v3.0 milestone Jan 21, 2016
@jsor
Copy link
Member Author

jsor commented Jan 21, 2016

I'm planning to add this to 3.0.

@jsor jsor merged commit 0865eac into master May 3, 2016
@jsor jsor deleted the global-queue branch May 3, 2016 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perhaps there is an iterative approach instead of recursive?
5 participants