refactor async / Promise loop(s)

[tomholub]

There's seems to be a bit of silliness in the way the following (and potentially a few more) piece of code is written at https://github.com/openpgpjs/openpgpjs/blob/master/src/key.js#L563

  await Promise.all(users.map(async function (a) {
    return a.user.revoked || a.user.isRevoked(primaryKey, a.selfCertification, null, date);
  }));

Firstly, async keyword is not needed there (may even affect error propagation as written - unsure).

  await Promise.all(users.map(function (a) {
    return a.user.revoked || a.user.isRevoked(primaryKey, a.selfCertification, null, date);
  }));

Secondly, what does it even do? It seems to want to make sure to go through the isRevoked (unless already known to be revoked) promisified function before proceeding below. If that's the case, I think the intent could be made a bit clearer with:

  for(const a of users.filter(a => !a.user.revoked)) { // evaluate revocations if any
    await a.user.isRevoked(primaryKey, a.selfCertification, null, date);
  }

I'm writing it here instead of making a PR because I'm unsure if there are other motivations. One would think if this is cpu bound and not IO bound, Promise.all should not bring a performance boost as opposed to doing it sequentially? (not tested)

[twiss]

If this is cpu bound and not IO bound, Promise.all should not bring a performance boost as opposed to doing it sequentially? (not tested)

Benchmarks are of course always welcome, but the theory behind why this could be faster is that user.isRevoked calls packet.signature.verify, which could use Web Crypto to hash the data, which could use multiple cores. (To be fair, for the specific code you quoted this is probably unlikely to actually happen, as the default config.min_bytes_for_web_crypto is probably too high for it.)

Nevertheless, this code pattern is used in many other places in the codebase, to indicate that we want to evaluate multiple async functions, and that they don't have to wait for each other. I'd argue that that information by itself is valuable to have, to make the intention of the code clear. Also, if we ever somehow implement signature verification in Web Crypto, we get a performance boost for free here.

Finally, it might make this code clearer to define a helper function for this, something like:

  await util.each(users, a => {
    return a.user.revoked || a.user.isRevoked(primaryKey, a.selfCertification, null, date);
  });

  util.each = (list, fn) => Promise.all(list.map(fn));

This might then make it less explicit that the intention was to execute them concurrently, though.

[tomholub]

Got it, there may be a performance benefit when using native crypto, if the engine is able to utilize more than one core (for native stuff), which it technically could. That's fair.

How about a pattern where the a.user.revoked is moved inside a.user.isRevoked and thus doesn't have to be checked explicitly outside? That would clean it up:

await Promise.all(users.map(a => a.user.isRevoked(primaryKey, a.selfCertification, null, date)));

(just asking naively)

[twiss]

Yeah, I think that would be a good improvement 👍