Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Prevent error responses wedging group request concurrency limit #1867

Merged
merged 3 commits into from
May 2, 2018

Conversation

lukebarnard1
Copy link
Contributor

@lukebarnard1 lukebarnard1 commented May 1, 2018

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, this still feels a bit dicey since we don't know for sure if the exception happened before or after the item started processing, so we could end up decrementing without ever incrementing.

This would feel better if the increment & decrement were in symmetrical places, eg:

ongoingRequestCount++;
try {
    resolve();
} finally {
    ongoingRequestCount--;
}

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr May 1, 2018
Make sure that we only catch errors that are a result of
calling fn()
@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 May 2, 2018
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks better. Also, you could just return the result directly - the finally block will always be run. Ideally this would also handle exceptions out of these functions by calling the appropriate reject function for the promise, as this will throw exceptions back to a different part of code if a queued request fails. Since they're all basically similar web requests this probably doesn't matter too much.

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr May 2, 2018
@lukebarnard1
Copy link
Contributor Author

TIL the finally get's called even if you return in the try!

Ideally this would also handle exceptions out of these functions by calling the appropriate reject function for the promise

Not so, calling reject on that promise would indicate a failure in the backlogging itself. Errors resulting from calling fn are caught and are thrown so that the stack trace leads to the original call to limitConcurrency.

@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 May 2, 2018
@dbkr
Copy link
Member

dbkr commented May 2, 2018

lgtm

@dbkr dbkr merged commit 3d478c3 into develop May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants