Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

ServiceApiClient: refactor construction, use thread-local instances #257

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

risicle
Copy link
Member

@risicle risicle commented Sep 10, 2024

Tangential to https://trello.com/c/ORGrd1jn/498-upgrade-docker-images-for-our-ecs-apps-to-debian-bookworm

This app uses eventlet, so ServiceApiClient use really needs to be made thread-safe because of its internal requests.Session.


🚨⚠️ This will be deployed automatically all the way to production when you click merge ⚠️🚨

For more information, including how to check this deployment on preview or staging first before it goes to production, see our team wiki section on deployment

@risicle risicle force-pushed the ris-service-api-client-thread-safe branch 3 times, most recently from ae535eb to 89339c2 Compare September 16, 2024 15:00
newer versions of the internal api_client are not thread-safe,
so we need to keep thread-local copies of this using
LazyLocalGetter if we want to use them
can switch to a non-thread-safe version now that we use
per-thread copies
@risicle risicle force-pushed the ris-service-api-client-thread-safe branch from 89339c2 to e51c1eb Compare September 30, 2024 11:58
Copy link
Contributor

@CrystalPea CrystalPea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of this is new to me - I read up on the constructs you used and I think I understand - your changes will make it safer for our code to run concurrently - but if you need someone who could spot problems with this PR, I'm afraid I'm out of my depth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants