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

changes to make app.current_request thread safe #1358

Closed

Conversation

jtetrault
Copy link

  • fixes race conditions that can occur when chalice is being run locally

Related Issue: #759

Wraps app.current_request access in a dict that is automatically keyed by the current thread's id, with each thread getting its own app.current_request.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

Merging #1358 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1358      +/-   ##
==========================================
+ Coverage   96.11%   96.12%   +0.01%     
==========================================
  Files          28       28              
  Lines        5324     5342      +18     
  Branches      682      682              
==========================================
+ Hits         5117     5135      +18     
  Misses        136      136              
  Partials       71       71              
Impacted Files Coverage Δ
chalice/local.py 99.40% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2eac21...33e7702. Read the comment docs.

@jamesls
Copy link
Member

jamesls commented Mar 4, 2020

Thanks for the pull request! Reading over the discussion in #759, I'd prefer to make this change in the local mode code because it only affects local mode.

@jtetrault jtetrault force-pushed the thread-safe-app-current-request branch from ba6e18c to d6eaabf Compare March 5, 2020 20:36
@jtetrault
Copy link
Author

Thanks for the pull request! Reading over the discussion in #759, I'd prefer to make this change in the local mode code because it only affects local mode.

That makes sense. I made some changes so that only code running in local mode will be affected. Unfortunately I had to resort to a bit of a hack because the Chalice instance that gets passed around is instantiated in the user's app.py module, so there is no chance to subclass it properly.

@jamesls
Copy link
Member

jamesls commented Mar 5, 2020

Thanks for the update. It looks like this failed the travis build so we'll need to get that resolved before we can merge.

@jtetrault jtetrault force-pushed the thread-safe-app-current-request branch 3 times, most recently from 331616b to a2e24e6 Compare March 6, 2020 01:06
@jtetrault
Copy link
Author

I've resolved most of the mypy issues, but I think the one remaining is actually a known bug with mypy.
python/mypy#4125

@jtetrault jtetrault force-pushed the thread-safe-app-current-request branch from a2e24e6 to ef3ce1f Compare March 6, 2020 15:54
@jamesls
Copy link
Member

jamesls commented Mar 9, 2020

Thanks for updating. I'd still like to see the travis build passing before merging so I'm open to ideas on how to do that. What if instead of subclassing we just had LocalChalice take a reference to the original Chalice object and override the current_request property but then delegating everything else via getattr(). Maybe that could work?

@jtetrault
Copy link
Author

Yeah, I had considered that approach as well. I can give that a try. I am not sure if mypy will be alright with it.

@jtetrault jtetrault force-pushed the thread-safe-app-current-request branch 2 times, most recently from 0f5f3b3 to a3b1897 Compare March 10, 2020 16:53
…locally

- fixes race conditions that can occur when chalice is being run locally
  and it handling multiple concurrent requests
@jtetrault jtetrault force-pushed the thread-safe-app-current-request branch from a3b1897 to 33e7702 Compare March 10, 2020 16:59
@jtetrault
Copy link
Author

Looks like things are passing now. Thanks for the suggestion! I added a basic unittest as well to satisfy codecov.

Copy link
Member

@jamesls jamesls left a comment

Choose a reason for hiding this comment

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

Thanks for updating! Looks good to me.

jamesls added a commit that referenced this pull request Mar 11, 2020
PR #1358.

* thread-safe-app-current-request:
  Make current_request thread safe for chalice local
@jamesls
Copy link
Member

jamesls commented Mar 11, 2020

Merged in 417e73e

@jamesls jamesls closed this Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants