Retry configuration is ignored after call to get_available_regions()

[nielslaukens]

I'm having trouble to use a non-default retry-configuration. After some debugging, I've encountered some unexpected behaviour that I consider a bug.

Code to reproduce the issue:

import botocore.session
from botocore.config import Config

# Any valid Elastic Beanstalk Environment ARN to query
arn = "TODO: FILL IN"


botocore_session = botocore.session.get_session()

# When this call is made, retry-config is ignored...
_ = botocore_session.get_available_regions('elasticbeanstalk')

client = botocore_session.create_client(
    'elasticbeanstalk',
    region_name="eu-west-1",
    config=Config(retries={'max_attempts': 2}),
)

while True:
    res = client.list_tags_for_resource(
        ResourceArn=arn,
    )
    if res['ResponseMetadata']['RetryAttempts'] > 2:
        print(res)
        break

The code needs to run with valid credentials configured (I used environment variables for my tests, but I don't thing it matters). Also, you need to fill in a valid ARN of an Elastic Beanstalk Environment to query. (I think the bug is not related to Beanstalk, but that is what I've used to debug the issue on).

The expected behaviour is for this code to hammer the API, and hit the Throttle limit. The list_tags_for_resource() call will then raise a ClientError explaining the issue.

The actual behaviour is that this code enters the final if, and prints out a successfull response that was retried more than 2 times, while the config is set to try at most 2 times.

Commenting out the get_available_regions() call, yields the expected behaviour. So it seams that this call changes some internal state of the session that causes later issues.

[JordonPhillips]

Interesting. I can reproduce. Looking into it.

[JordonPhillips]

Looks like the problem is that get_available_regions calls get_service_datawhich ends up
emitting an event that results in retry handlers being registered which don't respect the config. We'll need to take a closer look to see what the impact of removing that would be

[jamesls]

Just some additional info on the comment in the get_service_data handler

• There's one operation specific configuration in our _retry.json file (kinesis DescribeStream) (https://github.com/boto/botocore/blob/develop/botocore/data/_retry.json) We could just make this a __default__ for kinesis as LimitExceededException doesn't seem specific to DescribeStream.
• The CLI uses clients to make API calls so we'd run into the same problem if we ever decide to expose something like max_attempts in ~/.aws/config. get_service_data() is used in the CLI just to power the command tables and CLI parser.

A couple of other options we have if we don't want to remove the retry handler registration in handlers.py:

• Both handler.py and client.py use the same unique id so we can call unregister() in the client before we register() our handler.
• We could use a different unique_id in the client.py and use register_first().

However, my preference would be to remove the retry code in handlers.py. Operation level retries already don't work on clients, so to be affected you'd have to have a custom _retry.json that had operation specific retries, ensure it always gets loaded first via some call (even if implicitly called) to get_service_data(). Seems like we'd be fixing a consistency issue here.

What do you all think?

[jamesls]

The fix to remove the retry handler in handlers.py was pretty simple so I went ahead and put a PR together (#1719). Probably needs a bit more investigation but the tests are passing and seems to be working ok.