Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid a KeyError when a ComponentLocator is being called concurrently #2985

Merged
merged 1 commit into from
Jul 30, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Jul 18, 2023

The symptom is e.g. KeyError: 'credential_provider', detailed in e.g. #1246 (comment)

As detailed in the comment, even if the component factory gets called twice, cleaning the deferred up should not cause a KeyError.

@akx akx force-pushed the avoid-deferred-del-keyerror branch from 8df281b to bcb1f8e Compare July 18, 2023 10:30
@nateprewitt
Copy link
Contributor

Hi @akx, thanks for the PR. This seems like a reasonable thing to fix, but we should caveat that this case should never be hit under documented usage. Sessions are not thread safe and client creation shouldn't be done in parallel using the same session. We have the details on usage of Sessions around parallelization primitives here.

@akx
Copy link
Contributor Author

akx commented Jul 18, 2023

@nateprewitt I kind of understand that, but it really isn't very obvious that you should not use boto3.client() (that uses the implicit shared default session) from more than one thread.

Maybe the default session should be thread-local or plain refuse to work between threads if it's not actually supported?

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2023

Codecov Report

Patch coverage: 50.00% and no project coverage change.

Comparison is base (59edb73) 93.36% compared to head (bcb1f8e) 93.37%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2985   +/-   ##
========================================
  Coverage    93.36%   93.37%           
========================================
  Files           65       65           
  Lines        13841    13844    +3     
========================================
+ Hits         12923    12927    +4     
+ Misses         918      917    -1     
Impacted Files Coverage Δ
botocore/session.py 97.20% <50.00%> (-0.45%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nateprewitt
Copy link
Contributor

I think we cover this to some extent in the Session section and we note that boto3.client uses the default session in the docstring, but I agree it could be easier to find. I'll open an issue for that and merge this to help in the cases this is missed.

@nateprewitt nateprewitt merged commit 22ca528 into boto:develop Jul 30, 2023
@nateprewitt
Copy link
Contributor

#2991 created for reference.

@akx akx deleted the avoid-deferred-del-keyerror branch July 30, 2023 17:33
aws-sdk-python-automation added a commit that referenced this pull request Jul 31, 2023
* release-1.31.16:
  Bumping version to 1.31.16
  Update to latest partitions and endpoints
  Update to latest models
  fix: random crashes in credentials.py (#2946)
  Avoid a KeyError when a ComponentLocator is being called concurrently (#2985)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants