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

Execute requests in batches #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

danmana
Copy link

@danmana danmana commented Apr 14, 2019

Execute requests in batches to avoid high memory/cpu usage.

axios.all executes all requests in parallel, which can take up a lot of memory

@Pinjasaur Pinjasaur added the help wanted Extra attention is needed label Apr 20, 2019
@Pinjasaur
Copy link
Owner

Hi @danmana,

Thanks for the PR. I didn't want to leave this unattended too long — I've been busy outside of GitHub with the end of the semester rolling in.

I've checked this out locally a bit and it's a great idea. Nice work implementing it — puts my Promise knowledge to shame. ;)

The main issue I'm having is sometimes requests just don't get a response within Axios. Before, when I was dumping out all requests at once, I would just let it run until I there hadn't been any new responses & then Ctrl + C it. With this PR that logic wouldn't hold up since each batch of requests needs to resolve before the next one starts.

The (seemingly) obvious solution to this would be to use the timeout option within Axios. The problem I've been having is it's difficult to choose a sane value, particularly depending on how many domains & the batch size. Further, there seems to be some inconsistencies that I haven't gotten to the bottom of yet, such as the stats.total value being greater than the number of total requests. This should never be the case since that value is incremented for each request as the first statement in the .then() and .catch(), respectively.

The bottom line is I'm not comfortable merging this in until I can figure out what's going on and make sure it works as expected. If you have any thoughts on the matter I'd love to hear. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants