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

Use request-local config objects instead of singletons #1491

Draft
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@fredreichbier
Copy link
Member

fredreichbier commented Mar 7, 2019

This implements the flow described in #1132 for the config class (policies are still missing). Essentially, we get rid of the ConfigClass singleton. Instead,

  • each app has a SharedConfigClass object that is shared among threads. It holds the current config state and a timestamp.
  • each incoming request checks if the shared config should be updated according to the PI_CHECK_RELOAD_CONFIG setting. If yes, the shared config is updated. Afterwards, the request clones the current config state into a request-local LocalConfigClass object, i.e. concurrent config changes by other threads are not automatically propagated into this object. Functions like save_config_timestamp invalidate the request-local config, so that the request can afterwards clone the current config state again and work with the updated config.

TODO:

  • Check that this doesn't result in nasty issues with config changes during a request
  • Implement the flow for policies too
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #1491 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1491      +/-   ##
==========================================
+ Coverage    96.8%   96.81%   +0.01%     
==========================================
  Files         144      144              
  Lines       17250    17276      +26     
==========================================
+ Hits        16699    16726      +27     
+ Misses        551      550       -1
Impacted Files Coverage Δ
privacyidea/api/ttype.py 95.55% <100%> (ø) ⬆️
privacyidea/lib/resolver.py 100% <100%> (ø) ⬆️
privacyidea/api/auth.py 95.53% <100%> (ø) ⬆️
privacyidea/lib/config.py 95.27% <100%> (+0.59%) ⬆️
privacyidea/api/before_after.py 97.81% <100%> (ø) ⬆️
privacyidea/lib/realm.py 100% <100%> (ø) ⬆️
privacyidea/api/validate.py 100% <100%> (ø) ⬆️
privacyidea/models.py 98.91% <100%> (ø) ⬆️

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 c89c545...0cafc76. Read the comment docs.

Create a config object from the whole database, tables config,
resolver and realm.
"""
self._config_lock = threading.Lock()

This comment has been minimized.

@cornelinux

cornelinux Mar 19, 2019

Member

We need to check what this means for concurrent requests!
After all in 99.999% of all requests we only do reads! So basically it is not necessary to lock the object.

@@ -60,7 +60,7 @@ def before_request():
"""
This is executed before the request
"""
update_config_object()
invalidate_config_object()

This comment has been minimized.

@cornelinux

cornelinux Mar 19, 2019

Member

I do not understand this.
This reverses the logic. In the before_request we would fetch the config object. And at the beginning of the request, we need the config object. So I wonder why you would instead remove the config object from the request.

This comment has been minimized.

@cornelinux

cornelinux Mar 19, 2019

Member

Do I see this correctly, that the get_config_object does clone the SharedObject the first time it is needed (like in lib/realm.py).
So you refrain from loading the config in the before_request but only when and if it is needed?
Hm, I wonder if we should change this, because basically we always need to configobject. Even each authentication request needs to know the realm setting. I wonder if we could save some lines of code, if we move it to the before-request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.