What was this change for?

To fix a SpotBugs warning that complained about catching Exception when no exception was thrown and client.get() actually throws SdkClientException (and its subclasses), so changed this to more specific exception.

Should we do RuntimeException, instead? This would catch the same exceptions as before, but not upset spotbugs. Unless we want a potential behavior change?

This synchronized uses a different lock from the containing class's synchronized. Maybe we should just manually create a reentrant lock and share that?

Yes, Thanks for this, updated to use reentrant lock.

Instead of having a separate protected class and API for this, should it be a feature of Ec2MetadataClient? E.g. if they do Ec2MetadataClient.builder().build(), we use a shared HTTP client instead of a new one? We'd still use reference counting to close the shared HTTP client when no metadata clients are using it anymore.

That would remove the need for another protected API we need to maintain/remember to use, and improve the creation time of multiple Ec2MetadataClients for customers.

If we make Ec2MetadataClient.builder().build() calls use a shared HTTP client, then client.close() would only decrement the reference count instead of actually closing resources, this would be a behavior change for users right? They expect their client to be fully closed but it's not closing. Alternatively, if we kept user-created clients separate from internal provider clients, we'd need to add a new public API feature to Ec2MetadataClient for providers to use shared clients, but that would be a public API change and users could also access.

If we changed the default to use a shared client, how would that behavior change negatively affect the customer?

Should this just be a feature of Ec2MetadataClient as well? Ec2MetadataClient.builder().v1FallbackAllowed(true).build()

It would remove the duplicated code between the two implementations, make the usage of v1 something customers can choose to use in their own Ec2MetadataClients, and reduce the number of APIs we need to manage/remember to use.

That would be a public API change, and we only want IMDSv1 fallback for backward compatibility in internal providers so didn't go for that approach.

If we can reduce the number of protected APIs and simplify the code, it might be worth having v1 fallback be a public API.

I'm concerned about the duplication and additional protected surface area the current approach creates. Can you think of other ways to reduce it, without making a public API for enabling v1 fallback?

Darn, I see we already released something that uses this, so this is just backfilling it to mark it as protected. If we needed it to be protected, we should have moved it out of the internal package or ideally not have made it protected. Too late for that.

Yeah :(
Added the comment about it.