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

Introduce promise pool to limit number of concurrent executions #417

Merged
merged 1 commit into from Dec 8, 2018

Conversation

Projects
None yet
3 participants
@cdupuis
Copy link
Contributor

cdupuis commented Dec 7, 2018

No description provided.

@ddgenome
Copy link
Member

ddgenome left a comment

If ordering of the results matters, I think there is an issue.

* @param promises all promises to execute
* @param concurrent the max number of concurrent promise executions
*/
export async function executeAll<T>(promises: Array<Promise<T>>, concurrent: number = concurrentDefault()): Promise<T[]> {

This comment has been minimized.

@ddgenome

ddgenome Dec 7, 2018

Member

I did not know you could use a call a function to provide a default value. I guess because that is all actually managed at run time, not compile time.

This comment has been minimized.

@cdupuis

cdupuis Dec 7, 2018

Author Contributor

Yeah, the JS is interesting. It is just a function as the first statement of the function.


const results: T[] = [];
pool.addEventListener("fulfilled", (event: any) => {
results.push(event.data.result);

This comment has been minimized.

@ddgenome

ddgenome Dec 7, 2018

Member

Couldn't this approach return the results in a different order than the original array of promises? You could pretty easily test this.

This comment has been minimized.

@cdupuis

cdupuis Dec 7, 2018

Author Contributor

I added a different strategy to fix this. Good point.

pool.addEventListener("fulfilled", (event: any) => {
results.push(event.data.result);
});
await pool.start();

This comment has been minimized.

@ddgenome

ddgenome Dec 7, 2018

Member

Although it would probably just be easier scrap the listener and capture the return value here and return that. Or just return pool.start();

This comment has been minimized.

@cdupuis

cdupuis Dec 7, 2018

Author Contributor

pool.start() doesn't return the results. I thought that too but it isn't the case.

This comment has been minimized.

@ddgenome

ddgenome Dec 7, 2018

Member

That's terrible.

@ddgenome
Copy link
Member

ddgenome left a comment

Nice to handle rejections, but it is different behavior than what people might expect. As it is now, the returned array may contain Error objects.

results[results.indexOf(event.data.promise)] = event.data.result;
});
pool.addEventListener("rejected", (event: any) => {
results[results.indexOf(event.data.promise)] = event.data.error;

This comment has been minimized.

@ddgenome

ddgenome Dec 7, 2018

Member

Promise.all rejects immediately on any rejected promise. Are you intentionally choosing different behavior?

This comment has been minimized.

@cdupuis

cdupuis Dec 7, 2018

Author Contributor

I think in most of our usecaes you don’t want to actually exit early. Keep going and collect. Imho that makes more sense.

Maybe I should add the Promise<Array<T | Error>> as return value.

What you recommend?

This comment has been minimized.

@cdupuis

cdupuis Dec 8, 2018

Author Contributor

Turns out the es6-promise-pool rejects immediately on any rejected promise. So we are not changing the semantics here.

Actually I don't like it that much but that is for another day.

@cdupuis cdupuis force-pushed the promise-pool branch from 18eba5e to a0cb4d2 Dec 8, 2018

@cdupuis

This comment has been minimized.

Copy link
Contributor Author

cdupuis commented Dec 8, 2018

@ddgenome I'd like to merge this ahead of the release. Now that this is back to the old behaviour of rejecting on the first failure, I think it should be safe to merge, no?

@ddgenome
Copy link
Member

ddgenome left a comment

LGTM

@atomist-bot atomist-bot merged commit 445874d into master Dec 8, 2018

2 checks passed

license/cla Contributor License Agreement is signed.
Details
sdm/atomist/atomist-sdm Atomist Software Delivery Machine goals: all succeeded
Details
@atomist-bot

This comment has been minimized.

Copy link
Contributor

atomist-bot commented Dec 8, 2018

Pull request auto merged by Atomist.

  • 1 approved review by @ddgenome
  • 2 successful checks

[atomist:generated] [auto-merge:on-approve]

@atomist-bot atomist-bot deleted the promise-pool branch Dec 8, 2018

atomist-bot added a commit that referenced this pull request Dec 8, 2018

Changelog: #417 to added
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment