From aa38d9e2f4a32fd92be77a8458778aaa2edbc37f Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Tue, 30 Jan 2024 16:09:57 +0530 Subject: [PATCH 1/8] refactor: pipeline wherever possible, optimize calls Signed-off-by: Akhil Narang --- frappe/cache_manager.py | 34 +++++++++---------- frappe/desk/notifications.py | 11 +++---- frappe/utils/redis_wrapper.py | 61 +++++++++++++++++++++++++++++------ frappe/website/utils.py | 30 ++++++++--------- 4 files changed, 86 insertions(+), 50 deletions(-) diff --git a/frappe/cache_manager.py b/frappe/cache_manager.py index 960c72f042..ed6955224c 100644 --- a/frappe/cache_manager.py +++ b/frappe/cache_manager.py @@ -82,13 +82,11 @@ def clear_user_cache(user=None): clear_notifications(user) if user: - for name in user_cache_keys: - frappe.cache.hdel(name, user) + frappe.cache.hdel_names(user_cache_keys, user) frappe.cache.delete_keys("user:" + user) clear_defaults_cache(user) else: - for name in user_cache_keys: - frappe.cache.delete_key(name) + frappe.cache.delete_key(user_cache_keys) clear_defaults_cache() clear_global_cache() @@ -103,17 +101,15 @@ def clear_global_cache(): clear_doctype_cache() clear_website_cache() - frappe.cache.delete_value(global_cache_keys) - frappe.cache.delete_value(bench_cache_keys) + frappe.cache.delete_value(global_cache_keys + bench_cache_keys) frappe.setup_module_map() def clear_defaults_cache(user=None): if user: - for p in [user] + common_default_keys: - frappe.cache.hdel("defaults", p) + frappe.cache.hdel("defaults", [user] + common_default_keys) elif frappe.flags.in_install != "frappe": - frappe.cache.delete_key("defaults") + frappe.cache.delete_value("defaults") def clear_doctype_cache(doctype=None): @@ -128,15 +124,14 @@ def clear_doctype_cache(doctype=None): def _clear_doctype_cache_from_redis(doctype: str | None = None): from frappe.desk.notifications import delete_notification_count_for - for key in ("is_table", "doctype_modules"): - frappe.cache.delete_value(key) - - def clear_single(dt): - frappe.clear_document_cache(dt) - for name in doctype_cache_keys: - frappe.cache.hdel(name, dt) + to_del = ["is_table", "doctype_modules"] if doctype: + + def clear_single(dt): + frappe.clear_document_cache(dt) + frappe.cache.hdel_names(doctype_cache_keys, dt) + clear_single(doctype) # clear all parent doctypes @@ -157,9 +152,10 @@ def _clear_doctype_cache_from_redis(doctype: str | None = None): else: # clear all - for name in doctype_cache_keys: - frappe.cache.delete_value(name) - frappe.cache.delete_keys("document_cache::") + to_del += doctype_cache_keys + to_del += frappe.cache.get_keys("document_cache::") + + frappe.cache.delete_value(to_del) def clear_controller_cache(doctype=None): diff --git a/frappe/desk/notifications.py b/frappe/desk/notifications.py index 3ec0338e83..ac17f90006 100644 --- a/frappe/desk/notifications.py +++ b/frappe/desk/notifications.py @@ -72,7 +72,7 @@ def get_notifications_for_doctypes(config, notification_count): except frappe.PermissionError: frappe.clear_messages() pass - # frappe.msgprint("Permission Error in notifications for {0}".format(d)) + # frappe.msgprint("Permission Error in notifications for {0}".format(d)) except Exception as e: # OperationalError: (1412, 'Table definition has changed, please retry transaction') @@ -149,11 +149,10 @@ def clear_notifications(user=None): for_module = list(config.get("for_module")) if config.get("for_module") else [] groups = for_doctype + for_module - for name in groups: - if user: - frappe.cache.hdel("notification_count:" + name, user) - else: - frappe.cache.delete_key("notification_count:" + name) + if user: + frappe.cache.hdel_names([f"notification_count:{name}" for name in groups], user) + else: + frappe.cache.delete_value([f"notification_count:{name}" for name in groups]) def clear_notification_config(user): diff --git a/frappe/utils/redis_wrapper.py b/frappe/utils/redis_wrapper.py index 8128341edb..b94ea5ecf8 100644 --- a/frappe/utils/redis_wrapper.py +++ b/frappe/utils/redis_wrapper.py @@ -238,22 +238,65 @@ class RedisWrapper(redis.Redis): self.hset(name, key, value, shared=shared) return value - def hdel(self, name, key, shared=False): + def hdel( + self, + name: str, + keys: str | list | tuple, + shared=False, + pipeline: redis.client.Pipeline | None = None, + ): + """ + A wrapper around redis' HDEL command + + :param name: The hash name + :param keys: the keys to delete + :param shared: shared frappe key or not + :param pipeline: A redis.client.Pipeline object, if this transaction is to be run in a pipeline + """ _name = self.make_key(name, shared=shared) - if _name in frappe.local.cache: - if key in frappe.local.cache[_name]: - del frappe.local.cache[_name][key] - try: - super().hdel(_name, key) - except redis.exceptions.ConnectionError: - pass + if not isinstance(keys, (list, tuple)): + keys = (keys,) + + name_in_local_cache = _name in frappe.local.cache + + local_pipeline = False + + if pipeline is None: + pipeline = self.pipeline() + local_pipeline = True + + for key in keys: + if name_in_local_cache: + if key in frappe.local.cache[_name]: + del frappe.local.cache[_name][key] + try: + pipeline.hdel(_name, key) + except redis.exceptions.ConnectionError: + pass + + if local_pipeline: + pipeline.execute() + + def hdel_names(self, names: list | tuple, key: str): + """ + A function to call HDEL on multiple hash names with a common key, run in a single pipeline + + :param names: The hash names + :param key: The common key + """ + pipeline = self.pipeline() + for name in names: + self.hdel(name, key, pipeline=pipeline) + pipeline.execute() def hdel_keys(self, name_starts_with, key): """Delete hash names with wildcard `*` and key""" + pipeline = self.pipeline() for name in self.get_keys(name_starts_with): name = name.split("|", 1)[1] - self.hdel(name, key) + self.hdel(name, key, pipeline=pipeline) + pipeline.execute() def hkeys(self, name): try: diff --git a/frappe/website/utils.py b/frappe/website/utils.py index a92b65415f..0e4916023e 100644 --- a/frappe/website/utils.py +++ b/frappe/website/utils.py @@ -10,7 +10,6 @@ import yaml from werkzeug.wrappers import Response import frappe -from frappe import _ from frappe.model.document import Document from frappe.utils import ( cint, @@ -30,14 +29,13 @@ CLEANUP_PATTERN_3 = re.compile(r"(-)\1+") def delete_page_cache(path): - frappe.cache.delete_value("full_index") - groups = ("website_page", "page_context") + groups = ["website_page", "page_context"] if path: - for name in groups: - frappe.cache.hdel(name, path) + frappe.cache.hdel_names(groups, path) + frappe.cache.delete_value("full_index") else: - for name in groups: - frappe.cache.delete_key(name) + groups.append("full_index") + frappe.cache.delete_value(groups) def find_first_image(html): @@ -363,25 +361,24 @@ def clear_cache(path=None): :param path: (optional) for the given path""" from frappe.website.router import clear_routing_cache - for key in ( + clear_routing_cache() + + keys = [ "website_generator_routes", "website_pages", "website_full_index", "languages_with_name", "languages", - ): - frappe.cache.delete_value(key) + "website_404", + ] - clear_routing_cache() - - frappe.cache.delete_value("website_404") if path: frappe.cache.hdel("website_redirects", path) delete_page_cache(path) else: clear_sitemap() frappe.clear_cache("Guest") - for key in ( + keys += [ "portal_menu_items", "home_page", "website_route_rules", @@ -389,8 +386,9 @@ def clear_cache(path=None): "website_redirects", "page_context", "website_page", - ): - frappe.cache.delete_value(key) + ] + + frappe.cache.delete_value(keys) for method in frappe.get_hooks("website_clear_cache"): frappe.get_attr(method)(path) From 762290db9a3301bf98d943714f36c314457d1686 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Wed, 31 Jan 2024 18:12:15 +0530 Subject: [PATCH 2/8] chore: don't delete keys that aren't used Signed-off-by: Akhil Narang --- frappe/utils/password.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/frappe/utils/password.py b/frappe/utils/password.py index f5f83cef1e..3e6a8614d1 100644 --- a/frappe/utils/password.py +++ b/frappe/utils/password.py @@ -12,7 +12,6 @@ from frappe.utils import cstr, encode Auth = Table("__Auth") - passlibctx = CryptContext( schemes=[ "pbkdf2_sha256", @@ -104,9 +103,7 @@ def check_password(user, pwd, doctype="User", fieldname="password", delete_track def delete_login_failed_cache(user): - frappe.cache.hdel("last_login_tried", user) frappe.cache.hdel("login_failed_count", user) - frappe.cache.hdel("locked_account_time", user) def update_password(user, pwd, doctype="User", fieldname="password", logout_all_sessions=False): From 7dddc9e4f16994f59a9d56c3dcedbab3d220bce6 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 1 Feb 2024 13:22:27 +0530 Subject: [PATCH 3/8] refactor(twofactor): reduce number of calls to redis Pipeline wherever possible, set expiry while saving value Signed-off-by: Akhil Narang --- frappe/twofactor.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/frappe/twofactor.py b/frappe/twofactor.py index c9263aa336..b5ddd080f0 100644 --- a/frappe/twofactor.py +++ b/frappe/twofactor.py @@ -96,16 +96,17 @@ def cache_2fa_data(user, token, otp_secret, tmp_id): pwd = frappe.form_dict.get("pwd") verification_method = get_verification_method() + pipeline = frappe.cache.pipeline() + # set increased expiry time for SMS and Email if verification_method in ["SMS", "Email"]: expiry_time = frappe.flags.token_expiry or 300 - frappe.cache.set(tmp_id + "_token", token) - frappe.cache.expire(tmp_id + "_token", expiry_time) + pipeline.set(tmp_id + "_token", token, expiry_time) else: expiry_time = frappe.flags.otp_expiry or 180 for k, v in {"_usr": user, "_pwd": pwd, "_otp_secret": otp_secret}.items(): - frappe.cache.set(f"{tmp_id}{k}", v) - frappe.cache.expire(f"{tmp_id}{k}", expiry_time) + pipeline.set(f"{tmp_id}{k}", v, expiry_time) + pipeline.execute() def two_factor_is_enabled_for_(user): From 886d4ffc253e1ba81dab60ba055a98162ae96a86 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 1 Feb 2024 13:25:51 +0530 Subject: [PATCH 4/8] refactor(boot): frappe.cache() -> frappe.cache Signed-off-by: Akhil Narang --- frappe/boot.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/boot.py b/frappe/boot.py index 45a7cfa21c..39b6a091ce 100644 --- a/frappe/boot.py +++ b/frappe/boot.py @@ -450,12 +450,12 @@ def get_marketplace_apps(): return request.json()["message"] try: - apps = frappe.cache().get_value(cache_key, get_apps_from_fc, shared=True) + apps = frappe.cache.get_value(cache_key, get_apps_from_fc, shared=True) installed_apps = set(frappe.get_installed_apps()) apps = [app for app in apps if app["name"] not in installed_apps] except Exception: # Don't retry for a day - frappe.cache().set_value(cache_key, apps, shared=True, expires_in_sec=24 * 60 * 60) + frappe.cache.set_value(cache_key, apps, shared=True, expires_in_sec=24 * 60 * 60) return apps From 7c82115f5b2cdce94b8165aa9f43b7b58bcb29ff Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 1 Feb 2024 13:30:45 +0530 Subject: [PATCH 5/8] refactor(redis): DEL -> UNLINK This is beneficial since unlink is non blocking, it will run in a different thread if the data passed is large https://redis.io/commands/unlink/ Signed-off-by: Akhil Narang --- frappe/utils/redis_wrapper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/utils/redis_wrapper.py b/frappe/utils/redis_wrapper.py index b94ea5ecf8..da5fda88e7 100644 --- a/frappe/utils/redis_wrapper.py +++ b/frappe/utils/redis_wrapper.py @@ -144,7 +144,7 @@ class RedisWrapper(redis.Redis): frappe.local.cache.pop(key, None) try: - self.delete(*keys) + self.unlink(*keys) except redis.exceptions.ConnectionError: pass From 94365a59bfb91b43b3364170049b64b850a4ad80 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 1 Feb 2024 17:43:31 +0530 Subject: [PATCH 6/8] chore(utils/caching): drop unnecessary indent Signed-off-by: Akhil Narang --- frappe/utils/caching.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/frappe/utils/caching.py b/frappe/utils/caching.py index 0093146c95..e29879e8b2 100644 --- a/frappe/utils/caching.py +++ b/frappe/utils/caching.py @@ -154,11 +154,10 @@ def redis_cache(ttl: int | None = 3600, user: str | bool | None = None) -> Calla func_call_key = func_key + "::" + str(__generate_request_cache_key(args, kwargs)) if frappe.cache.exists(func_call_key): return frappe.cache.get_value(func_call_key, user=user) - else: - val = func(*args, **kwargs) - ttl = getattr(func, "ttl", 3600) - frappe.cache.set_value(func_call_key, val, expires_in_sec=ttl, user=user) - return val + val = func(*args, **kwargs) + ttl = getattr(func, "ttl", 3600) + frappe.cache.set_value(func_call_key, val, expires_in_sec=ttl, user=user) + return val return redis_cache_wrapper From 5343bcb75675f87c0300b7bc2b8adf364d4b30d3 Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Fri, 2 Feb 2024 16:49:59 +0530 Subject: [PATCH 7/8] fix(redis_wrapper/hdel): don't pipeline if we can avoid it For a single deletion it doesn't make much sense, so just directly delete and return Signed-off-by: Akhil Narang --- frappe/utils/redis_wrapper.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/frappe/utils/redis_wrapper.py b/frappe/utils/redis_wrapper.py index da5fda88e7..99cd06bfab 100644 --- a/frappe/utils/redis_wrapper.py +++ b/frappe/utils/redis_wrapper.py @@ -255,11 +255,17 @@ class RedisWrapper(redis.Redis): """ _name = self.make_key(name, shared=shared) - if not isinstance(keys, (list, tuple)): - keys = (keys,) - name_in_local_cache = _name in frappe.local.cache + if not isinstance(keys, list | tuple): + if name_in_local_cache and keys in frappe.local.cache[_name]: + del frappe.local.cache[_name][keys] + if pipeline: + pipeline.hdel(_name, keys) + else: + super().hdel(_name, keys) + return + local_pipeline = False if pipeline is None: From c60e97410dd6495cf137b1400154212db469b97a Mon Sep 17 00:00:00 2001 From: Akhil Narang Date: Thu, 1 Feb 2024 18:43:19 +0530 Subject: [PATCH 8/8] chore(cache): add in some tests Signed-off-by: Akhil Narang --- frappe/tests/test_caching.py | 77 +++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_caching.py b/frappe/tests/test_caching.py index ccc44bbcfc..083cfa8a6b 100644 --- a/frappe/tests/test_caching.py +++ b/frappe/tests/test_caching.py @@ -2,6 +2,7 @@ import time from unittest.mock import MagicMock import frappe +from frappe.core.doctype.doctype.test_doctype import new_doctype from frappe.tests.test_api import FrappeAPITestCase from frappe.tests.utils import FrappeTestCase from frappe.utils.caching import redis_cache, request_cache, site_cache @@ -194,7 +195,7 @@ class TestDocumentCache(FrappeAPITestCase): self.test_value = frappe.generate_hash() def test_caching(self): - doc = frappe.get_cached_doc(self.TEST_DOCTYPE, self.TEST_DOCNAME) + frappe.get_cached_doc(self.TEST_DOCTYPE, self.TEST_DOCNAME) with self.assertQueryCount(0): doc = frappe.get_cached_doc(self.TEST_DOCTYPE, self.TEST_DOCNAME) @@ -243,5 +244,79 @@ class TestRedisWrapper(FrappeAPITestCase): frappe.cache.delete_keys(prefix) self.assertEqual(len(frappe.cache.get_keys(prefix)), 0) + def test_hash(self): + key = "test_hash" + + # Confirm that there's no data initially + exists = frappe.cache.exists(key) + self.assertFalse(exists) + + # Insert 5 key-value pairs + for i in range(5): + frappe.cache.hset(key, f"key_{i}", f"value_{i}") + + # Check that we have 5 values + values = frappe.cache.hgetall(key) + self.assertEqual(len(values), 5) + + # Check that each value matches + for i in range(5): + value = frappe.cache.hget(key, f"key_{i}") + self.assertEqual(value, f"value_{i}") + + # Check the keys themselves + keys = frappe.cache.hkeys(key) + for i in range(5): + self.assertIn(f"key_{i}".encode(), keys) + + # Delete a single key and check that we still have the remaining 4 + frappe.cache.hdel(key, "key_1") + values = frappe.cache.hgetall(key) + self.assertEqual(len(values), 4) + + # Delete 2 keys and check that we still have the remaining 2 + frappe.cache.hdel(key, ["key_2", "key_3"]) + values = frappe.cache.hgetall(key) + self.assertEqual(len(values), 2) + + # Delete the hash itself and confirm that there's no data + frappe.cache.delete_value(key) + exists = frappe.cache.exists(key) + self.assertFalse(exists) + + def test_user_cache_clear(self): + from frappe.cache_manager import user_cache_keys + + # Set some keys that a user's cache would usually have + user1 = frappe.utils.random_string(10) + user2 = frappe.utils.random_string(10) + for key in user_cache_keys: + frappe.cache.hset(key, user1, key) + frappe.cache.hset(key, user2, key) + + frappe.clear_cache(user=user1) + + # Check that the keys for user1 are gone + for key in user_cache_keys: + self.assertFalse(frappe.cache.hexists(key, user1)) + self.assertTrue(frappe.cache.hexists(key, user2)) + + def test_doctype_cache_clear(self): + from frappe.cache_manager import doctype_cache_keys + + # Set some keys that a user's cache would usually have + doctype1 = new_doctype(frappe.utils.random_string(10)) + doctype2 = new_doctype(frappe.utils.random_string(10)) + for key in doctype_cache_keys: + frappe.cache.hset(key, doctype1.name, key) + frappe.cache.hset(key, doctype2.name, key) + + frappe.clear_cache(doctype=doctype1.name) + + # Check that the keys for doctype1 are gone + for key in doctype_cache_keys: + self.assertFalse(frappe.cache.hexists(key, doctype1.name)) + self.assertTrue(frappe.cache.hexists(key, doctype2.name)) + def test_backward_compat_cache(self): self.assertEqual(frappe.cache, frappe.cache())