Automatic retry for DNS lookup

[jagoda]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 145

• 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 142:	0.0%
Covered Lines:	375
Relevant Lines:	375

---

💛 - Coveralls

[jagoda]

@mdlavin @Druotic any thoughts now that tests are passing?

[mdlavin]

It looks like some of this is implementing retry logic instead of using an existing library. Is there a reason to avoid some of the retry libraries we've used before?

[jagoda]

My thinking was twofold. First, using a promise-based library makes it a little more difficult/clunky to maintain the Node API interface. Second, using a dependency for a utility in an injected header like this could significantly increase the footprint of this change.

[jagoda]

@mdlavin any thoughts on this?

[mdlavin]

If it was me, I'd probably use an external library for the retry logic based on callbacks. I think I understand your points and I can appreciate the trade-offs. I'll approve with the code as is it.

[mdlavin]

Minor point, but a pattern I've used for callbacks in try/catch blocks is:

try {
  something
} catch (e) {
  callback(e);
  return;
}
callback(null, success)

The current code can cause a double callback if the callback(null, 'success!'); line throws an Error. That case becomes very hard to debug when it happens.

[jagoda]

Good point. I'll update this. Thanks.

[Druotic]

In the context of most requests, this seems like a very long time to wait to retry a dns lookup. Should this be a smaller value, or perhaps configurable?

[jagoda]

I considered this. The thing is that ENI cold starts are quite slow so I don't expect that retrying more frequently is very likely to succeed. I'd rather not make this configurable as I think it adds undo complexity and I don't see any value in changing this parameter between projects.