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

Avoid loading the request stream to retrieve context #8535

Merged
merged 1 commit into from Jun 20, 2023

Conversation

dfangl
Copy link
Member

@dfangl dfangl commented Jun 20, 2023

Issue

We are currently loading the flask request for a thread every time we call get_request_context(), even though we might find it in the thread local variable anyway.

This leads to the request streams being read, if you access methods like "get_region_from_request_context`, and the streams not being available anymore.

Solution

Instead of loading both, and then iterating, we will now load while iterating. This means, if the threadlocal request context is set, we will not call get_flask_request_for_thread, and therefore not load the request.

While this is still implicit, and it is still possibly loading the request into a flask request, it provides a reasonable improvement until we can get rid of the flask context one and for all.

@dfangl dfangl requested a review from thrau June 20, 2023 16:47
@thrau thrau added the semver: patch Non-breaking changes which can be included in patch releases label Jun 20, 2023
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

that's a wicked find! LGTM

@coveralls
Copy link

Coverage Status

coverage: 82.792% (+0.02%) from 82.769% when pulling 2edd69a on avoid-stream-loading into a83e15f on master.

@github-actions
Copy link

LocalStack Community integration with Pro

       2 files         2 suites   1h 24m 13s ⏱️
2 101 tests 1 815 ✔️ 286 💤 0
2 102 runs  1 815 ✔️ 287 💤 0

Results for commit 2edd69a.

@dfangl dfangl merged commit 5da0fd3 into master Jun 20, 2023
26 of 27 checks passed
@dfangl dfangl deleted the avoid-stream-loading branch June 20, 2023 18:40
@dfangl dfangl added this to the 2.2 milestone Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants