3

My code in order to create multiple placements in an online service that has a 60 write constraint per minute:

  placementsToAdd.forEach((placement, index) => {
    setTimeout(() => {
      options.url = `https://api.company.com/placement?publisher_id=${existingPub ? existingPub : placementsJson[0].PublisherId}&site_id=${placement.siteId}`
      options.body = `{"placement":{"name":"${placement.placement}"}}`
      request(options, callback);  
    },1000 * (index + 1))
  })

It works this way but I am concerned about the wait time if there are a list of placements of 2000 or 3000 at one time, the wait time might be excessively long.

Is there a better way to refactor this code in order to get my requests built one per second no matter what? Without that "* (index + 1)," it seems to keep trying to build all at once hitting the wall after 60.

I've tried to use promises and async await (which is new to me) but it doesn't seem to change the behavior.

Thanks!

As requested, show how I've tried to use promises with this code:

  async function createThePlacements() {
    let promise = new Promise((resolve, reject) => {
      for (let i = 0; i < placementsToAdd.length; i++) {
        setTimeout(() => {
          options.url = `https://api.company.com/placement?publisher_id=${existingPub ? existingPub : placementsJson[0].PublisherId}&site_id=${placementsToAdd[i].siteId}`
          options.body = `{"placement":{"name":"${placementsToAdd[i].placement}"}}`
          request(options, callback);  
        },1000)
      }
    });

    let result = await promise; // pause till the promise resolves 
    console.log('result - ', result);
  }

  createThePlacements();

So, bit of a disclaimer - as mentioned, I've never used Async Await before so reading up to try to understand how it works as well. This seems to be the syntax but my result doesn't seem to be anything at the moment but the code also continues to do what it's supposed to do, just trying to make all the calls in my test of 300 all at once.

Also, of note, i have a resolve inside the callback of the request call. It resolves so even the next parts of my app finish up all the way to the end. That's why I don't have a reject or resolve here.

nyhunter77
  • 614
  • 2
  • 7
  • 19
  • Is this in Node.js? – Ry- Apr 13 '19 at 19:03
  • 1
    Can you show us how you tried to use promises, please? Notice that [you can't use `forEach` with them](https://stackoverflow.com/q/37576685/1048572). – Bergi Apr 13 '19 at 19:04
  • Yep, it's node and I've edited my question to show my last try with promises / async / await. Appreciate it – nyhunter77 Apr 13 '19 at 19:18

2 Answers2

4

How do you run a setTimeout once a second inside a forEach loop?

The most straightforward approach would be:

const wait = ms => new Promise(resolve => setTimeout(resolve, ms));

for (const placement of placementsToAdd) {
  const options = {...};
  request(options, callback);  
  await wait(1000);
}

await works predictably inside plain for-loops, not inside forEach.

I haven't touched your callback but it would need to handle errors. More refactoring is possible.

The most significant improvement here, I think, is that we're not pushing requests ahead of time. This way we retain control, and should needs change or anything go haywire, we can break out of the loop without spamming the server for another minute.

jib
  • 40,579
  • 17
  • 100
  • 158
  • 2
    This worked really well, thanks! It seemed to be the easiest to implement and fit in with what I had already done in my example above. I was able to get it working and now confidently will know that even with 3000 placements to build, I won't be waiting longer and longer for the actions to finish. – nyhunter77 Apr 14 '19 at 05:11
0

The best option would be to have a request method that returns a Promise.

Then you could rewrite your code like this.

function sleep(ms) {
  return new Promise(resolve => setTimeout(resolve, ms));
}

function requestPlacement(placement) {
  const options = {...};
  return request(options);
}

async function requestAllPlacements(placements) {
  for(let i = 0; i < placements.length; i+=60) {
    if (i > 0) {
      // wait 1 minute
      await(sleep(60000));
    }

    await Promise.all(
      placements
        .slice(i, 60)
        .map(requestPlacement);
    );
  }
}
Olivier Boissé
  • 15,834
  • 6
  • 38
  • 56
  • There may be an API limit of 60 requests every minute, but if the site is behind a proxy server, there may be a hardcoded limit of requests within a small period of time that this could trigger as well. – Patrick Roberts Apr 13 '19 at 19:33
  • the author said **60 writes constraint per minute** – Olivier Boissé Apr 13 '19 at 19:36
  • Yes, as I stated I had read in the comment. That is an _API-enforced_ limit. Many servers are also behind some sort of DDoS protection, which typically have their own limits not necessarily disclosed in API documentation. – Patrick Roberts Apr 13 '19 at 19:41