fix: only replace refresh result if successful or current result is invalid

[shubha-rajan]

Change Description

Checklist

• Make sure to open an issue as a
  bug/issue
  before writing your code! That way we can discuss the change, evaluate
  designs, and agree on the general idea.
• Ensure the tests and linter pass
• Appropriate documentation is updated (if necessary)

Relevant issues:

• Fixes Only replace refresh result if refresh was successful or current result is invalid #550
• Fixes "java.io.IOException: Error writing request body to server" error during create ephemeral certificate  #528

[kurtisvg]

I think the logic we want here is:

On error:
     if current is invalid:
          replace current
     next = new new refresh

Since we know this refresh failed, we always want to schedule a new one. We just don't want it to replace "current" if we think "current' might still be good.

[kurtisvg]

We need to be careful here. There are at least two problems I see with the current code.

1. If "current" is in a bad state (e.i. error) it'll throw an (maybe unrelated) error and nothing past this will happen
2. if "current" is this thread (because we've already forced refresh), this will - well I'm not sure. Block forever?

[shubha-rajan]

I don't think we'd ever get into a position where current is still pending, since this callback happens after refreshFuture has completed. The first problem is something to think about though. how can we check the validity of the instance data here, without potentially throwing errors. Would a try/catch work, where we try getInstanceData and if we get an error, we replace current

[kurtisvg]

Yea, I think I would go with a try/catch around it. Just to clarify - you are sure that the callback will trigger after the future is completed?

[kurtisvg]

One thing to think about - I think we actually want the replace of current and next to happen before the future is returned. Otherwise you may introduce a race condition where we return, fail to connect, and force refresh before the callback is executed

[shubha-rajan]

Yeah, the callback will definitely trigger after the future is completed. I get what you're saying with the race condition, where a force refresh could be triggered before the callback, but how can we replace current before we know the future's result if our goal is to avoid replacing a valid InstanceData with an invalid one?

I think in the case where a force refresh is triggered before the callback, the worst that would happen is that we get an extra refresh attempt a minute later, since the rate limiter will delay the force refresh anyway. If the refresh attempt is successful, then we'd end up replacing a valid result with another (hopefully) valid result, or wait for another refresh attempt to fix it.

[kurtisvg]

To clarify, I think it's preferable if whatever the callback behavior is (replacing current or just rescheduling) before the future counts as "done" .

[kurtisvg]

Can you use [thenApply](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html#thenApply-java.util.function.Function-) for that?

[kurtisvg]

LGTM, minus my one nit. Can we make sure we test it in Cloud Run for an extended period of time (~24hours?) before merging?

[enocom]

After running this over the weekend, here's what I saw:

That's one runtime exception in several days, but that didn't result in a 500. This PR seems solid.

[shubha-rajan]

That's one runtime exception in several days, but that didn't result in a 500. This PR seems solid.

Good to know! I'll merge this then. Thanks!