From afc7c572e4ae6e3c7724ee9360ffb7d2b400b78c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= <bdgdlm@outlook.com>
Date: Mon, 1 Jul 2024 09:39:30 +0200
Subject: [PATCH] Update pyo3 to 0.22

This removes the `Clone` impl from `Py<T>` which we can work around by
keeping the GIL a bit longer in `log`. I believe that this has little to
no implication (or maybe a positive one actually due to the switch to
`clone_ref`).

However, this means that we have to remove `Clone` impl from `Logger`.

All of this is still compatible with `0.21` but still a breaking change.
---
 CHANGELOG.md |  5 +++++
 Cargo.toml   |  6 +++---
 src/lib.rs   | 44 ++++++++++++++++++++++++++++----------------
 3 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d0de112..3ac70f4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,8 @@
+# 0.11.0
+
+* Update to pyo3 0.22.
+* `Logger` not longer implements `Clone`
+
 # 0.10.0
 
 * Update to pyo3 0.21.
diff --git a/Cargo.toml b/Cargo.toml
index 9a99a0a..401e362 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "pyo3-log"
-version = "0.10.0"
+version = "0.11.0"
 authors = ["Michal 'vorner' Vaner <vorner@vorner.cz>"]
 description = "Logging bridge from pyo3 native extension to python"
 documentation = "https://docs.rs/pyo3-log"
@@ -18,10 +18,10 @@ rust-version = "1.56.0"
 arc-swap = "~1"
 # It's OK to ask for std on log, because pyo3 needs it too.
 log = { version = "~0.4.4", default-features = false, features = ["std"] }
-pyo3 = { version = ">=0.21, <0.22", default-features = false }
+pyo3 = { version = ">=0.21, <0.23", default-features = false }
 
 [dev-dependencies]
-pyo3 = { version = ">=0.21, <0.22", default-features = false, features = ["auto-initialize", "macros"] }
+pyo3 = { version = ">=0.21, <0.23", default-features = false, features = ["auto-initialize", "macros"] }
 
 # `pyo3-macros` is lying about the minimal version for its `syn` dependency.
 # Because we're testing with `-Zminimal-versions`, we need to explicitly set it here.
diff --git a/src/lib.rs b/src/lib.rs
index c2dbe3b..e13cc4a 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -217,28 +217,45 @@ impl Default for Caching {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 struct CacheEntry {
     filter: LevelFilter,
     logger: PyObject,
 }
 
-#[derive(Clone, Debug, Default)]
+impl CacheEntry {
+    fn clone_ref(&self, py: Python<'_>) -> Self {
+        CacheEntry {
+            filter: self.filter,
+            logger: self.logger.clone_ref(py),
+        }
+    }
+}
+
+#[derive(Debug, Default)]
 struct CacheNode {
     local: Option<CacheEntry>,
     children: HashMap<String, Arc<CacheNode>>,
 }
 
 impl CacheNode {
-    fn store_to_cache_recursive<'a, P>(&self, mut path: P, entry: CacheEntry) -> Arc<Self>
+    fn store_to_cache_recursive<'a, P>(
+        &self,
+        py: Python<'_>,
+        mut path: P,
+        entry: CacheEntry,
+    ) -> Arc<Self>
     where
         P: Iterator<Item = &'a str>,
     {
-        let mut me = self.clone();
+        let mut me = CacheNode {
+            children: self.children.clone(),
+            local: self.local.as_ref().map(|e| e.clone_ref(py)),
+        };
         match path.next() {
             Some(segment) => {
                 let child = me.children.entry(segment.to_owned()).or_default();
-                *child = child.store_to_cache_recursive(path, entry);
+                *child = child.store_to_cache_recursive(py, path, entry);
             }
             None => me.local = Some(entry),
         }
@@ -253,7 +270,7 @@ impl CacheNode {
 ///
 /// It can be either created directly and then installed, passed to other aggregating log systems,
 /// or the [`init`] or [`try_init`] functions may be used if defaults are good enough.
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct Logger {
     /// Filter used as a fallback if none of the `filters` match.
     top_filter: LevelFilter,
@@ -464,12 +481,12 @@ impl Logger {
         metadata.level() <= cache_filter && metadata.level() <= self.filter_for(metadata.target())
     }
 
-    fn store_to_cache(&self, target: &str, entry: CacheEntry) {
+    fn store_to_cache(&self, py: Python<'_>, target: &str, entry: CacheEntry) {
         let path = target.split("::");
 
         let orig = self.cache.load();
         // Construct a new cache structure and insert the new root.
-        let new = orig.store_to_cache_recursive(path, entry);
+        let new = orig.store_to_cache_recursive(py, path, entry);
         // Note: In case of collision, the cache update is lost. This is fine, as we simply lose a
         // tiny bit of performance and will cache the thing next time.
         //
@@ -497,7 +514,6 @@ impl Log for Logger {
     fn log(&self, record: &Record) {
         let cache = self.lookup(record.target());
 
-        let mut store_to_cache = None;
         if self.enabled_inner(record.metadata(), &cache) {
             Python::with_gil(|py| match self.log_inner(py, record, &cache) {
                 Ok(Some(logger)) => {
@@ -510,18 +526,14 @@ impl Log for Logger {
                                 LevelFilter::max()
                             }),
                     };
-                    store_to_cache = Some((logger, filter));
+
+                    let entry = CacheEntry { filter, logger };
+                    self.store_to_cache(py, record.target(), entry);
                 }
                 Ok(None) => (),
                 Err(e) => e.print(py),
             })
         }
-        // Note: no more GIL here. Not needed for storing to cache.
-
-        if let Some((logger, filter)) = store_to_cache {
-            let entry = CacheEntry { filter, logger };
-            self.store_to_cache(record.target(), entry);
-        }
     }
 
     fn flush(&self) {}