Skip to content

Conversation

@Suven
Copy link

@Suven Suven commented Oct 7, 2017

This PR introduces a new function throttle.

throttle behaves like all, but limits the number of concurrent requests by a passed parameter.

One use case for throttles are if you are sending a bunch of api-requests but don't want to hit rate-limits or if you want to have more control over your systems resources.

This PR is WIP as I need to:

  • also run the tests that pass for all
  • introduce tests that actually check if the concurrency-limit is being kept
  • update the projects readme to reflect this new function

Before doing that I wanted to check if this is in scope of this repo anyway. If you feel like this is out of scope, I will create another package for that.

The implementation is somewhat naive. Instead of creating a queue and starting another promise whenever one item finishes, I made a chain, where every promise has a succeeding one. That has the downside that the concurrency-limit may not always be used ideally. It is guaranteed to be not overused, but situations may exist, where you are (for example) only using 8 instead of 10 promises at the same time.

Any feedback / ideas / help is very welcome!

@jsor
Copy link
Member

jsor commented Oct 7, 2017

I'm not sure i understand the purpose of this function. Promises are placeholder for the results of an operation. So, once you have a promise, the operation has been already started.

You probably want to throttle the the number of started operations. In case of HTTP requests this for example the execution of a hypothetical request() function.

$requests = [
    function () {
        return request('https://reactphp.org');
    },
    function () {
        return request('https://example.com');
    },
    function () {
        return request('https://google.com');
    },
    function () {
        return request('https://php.net');
    }
];

throttle($requests, 2)->then(function (array $results) {
   // All 4 requests finished, but only 2 were executed in parallel
});

And btw, this again a perfect candidate for a third-party utility package. We had a similar discussion in #101 (comment) lately.

@clue
Copy link
Member

clue commented Oct 13, 2017

@Suven Thanks for filing this PR! I see where you're coming from and I like the idea of bringing this mechanism closer to the concept of promises 👍

From a technical perspective however, I have to agree with @jsor: Once you have a promise object, the operation has already been started. This mean that you should probably throttle the invocations instead.

I also think @jsor makes a valid point how creating this in a separate repository has the benefit that it's really easy to consume this for other packages and also how this allows for alternative throttle mechanisms to co-exist.

What do you think about this?

@Suven
Copy link
Author

Suven commented Oct 13, 2017

Thanks for your feedback @clue and @jsor!

Of course you are both right. I made this PR under the false assumption that Promises would be lazy be default, which they are not, so yeah, this does not make much sense right now 😅

I will have a look into the LazyPromise-Implementation, how Bluebird did it (since JS-Promises, in theory, have the same behaviour, but they implemented it anyway, and will then think about which path I will take with this.

Thanks again for your thoughts!

@Suven Suven closed this Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants