Skip to content

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