Open kgutwin opened 8 months ago
That's some impressive detective work! This seems like a real problem. I have two questions:
@cached_property
that uses a thread-local cache?Thanks for looking into it! I spent some time this morning trying to research answers to your questions:
flask.g
as you would probably want to use. The closest I found was cachetools which offers a customizable @cached()
decorator, although they don't seem to have a good @cached_property
replacement. It may be the most straightforward and transparent to just refactor the code using @property
and to store the result in flask.g
. authorized()
endpoint and making a secondary client call to an endpoint that calls the blueprint .authorized
property. I can submit a PR for that test case if you would like.
I noticed a concerning potential race condition today while using flask-dance via the Flask local dev server. Here's the setup:
flask_dance.contrib.github.authorized
in order to check if the session should be pulled from GitHub.I tried to trace the code paths within flask-dance and I think that the use of
@cached_property
on either the blueprint'ssession.token
or on the blueprint'ssession
itself is part of the problem. Even thoughflask_dance.contrib.github
is a LocalProxy tog.flask_dance_github
(which is equivalent togithub_bp.session
) that isn't sufficient for separation between threads --@cached_property
's cache is associated with the blueprint, which is global across all threads/requests.Basically, what I think is happening in my case is:
authorized()
blueprint endpointgithub_bp.session.token
github.authorized
github_bp.session.token
, which is cached by@cached_property
github.get("/user")
to fetch the full session infoauthorized()
blueprint endpoint concludes, and the session is set as expected to the original requestI've done a little testing of my app that is deployed in a test environment using gunicorn (which separates requests into processes). I haven't been able to reproduce the problem there yet, but because it's stochastic, I haven't fully ruled it out yet. Regardless, though, this seems like a concern as login sessions should never leak across threads.
Lastly, I tried a quick patch to take out the use of
@cached_property
forsession
andsession.token
, and it seemed to fix the issue -- I was unable to trigger the bad behavior even when the logs showed the possibility of a race condition.Should we consider removing
@cached_property
or replace it with something that uses a thread-local cache?