Replace lazy_static crate with std::sync::OnceLock

[tottoto]

std::sync::OnceLock can be used instead of lazy_static crate.

[adamreichold]

Not directly related, but the use of catch_unwind and resume_unwind in with_reset_proxy_vars appears superfluous? Why not just call test directly and let it unwind itself. The effect should be the same...

[tottoto]

I think this implementation would make the lock drop before panic which occurs when each test fails.

[adamreichold]

I don't see how: The guard is dropped during unwinding in either case. Directly when test unwinds through with_reset_proxy_vars or indirectly when the unwind is resumed by with_reset_proxy_vars. In both cases, the lock is released after the panic occurred. It basically must be equivalent because because the only between catch_unwind and resume_unwind is the comment.

[tottoto]

I think drop should be called before resume_unwind.

[adamreichold]

I think drop should be called before resume_unwind.

Do you mean that this already happens or that an explicit call to drop should be added? If it is the former, I do not think this is the case as resuming the unwinding does not change when Drop is called for objects on the current stack frame AFAIU.

[tottoto]

Do you have the reproduction which show that they behaves the same?

[adamreichold]

Not sure what you want to see but if you apply the following patch

diff --git a/src/request/proxy.rs b/src/request/proxy.rs
index d2c81d8..cb49302 100644
--- a/src/request/proxy.rs
+++ b/src/request/proxy.rs
@@ -226,6 +226,16 @@ where
 
     let _guard = LOCK.lock().unwrap();
 
+    struct ShowDrop;
+
+    impl Drop for ShowDrop {
+        fn drop(&mut self) {
+            println!("dropped");
+        }
+    }
+
+    let _show_drop = ShowDrop;
+
     env::remove_var("ALL_PROXY");
     env::remove_var("HTTP_PROXY");
     env::remove_var("HTTPS_PROXY");
@@ -240,6 +250,12 @@ where
     }
 }
 
+#[test]
+#[should_panic]
+fn when_with_reset_proxy_vars_drops() {
+    with_reset_proxy_vars(|| panic!("in the disco"));
+}
+
 #[test]
 fn test_proxy_from_env_all_proxy() {
     with_reset_proxy_vars(|| {

you'll get

running 1 test
thread 'request::proxy::when_with_reset_proxy_vars_drops' panicked at src/request/proxy.rs:256:30:
in the disco
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
dropped
test request::proxy::when_with_reset_proxy_vars_drops - should panic ... ok

which stays unchanged if you additionally apply

diff --git a/src/request/proxy.rs b/src/request/proxy.rs
index cb49302..6a60e08 100644
--- a/src/request/proxy.rs
+++ b/src/request/proxy.rs
@@ -241,13 +241,7 @@ where
     env::remove_var("HTTPS_PROXY");
     env::remove_var("NO_PROXY");
 
-    let result = std::panic::catch_unwind(test);
-
-    // teardown if ever needed
-
-    if let Err(ctx) = result {
-        std::panic::resume_unwind(ctx);
-    }
+    test();
 }
 
 #[test]

[tottoto]

Thanks, I have known that behavior. In my test, without manually dropping the guard before calling resume_unwind, when one of the tests (which are called in this helper function) fails for testing, the others (which are actually successful) will go failure as well.

[adamreichold]

Thanks, I have known that behavior. In my test, without manually dropping the guard before calling resume_unwind, when one of the tests (which are called in this helper function) fails for testing, the others (which are actually successful) will go failure as well.

But this already happens with the current code before and after this PR just due to poising, i.e. when one test fails and unwinds, the mutex is poisoned and acquiring it by the other fails (if they happen to run afterwards which is not guaranteed due to multi-threaded testing).

So what might want to do is

diff --git a/src/request/proxy.rs b/src/request/proxy.rs
index d2c81d8..9f18601 100644
--- a/src/request/proxy.rs
+++ b/src/request/proxy.rs
@@ -224,7 +224,7 @@ where
         static ref LOCK: Mutex<()> = Mutex::new(());
     };
 
-    let _guard = LOCK.lock().unwrap();
+    let guard = LOCK.lock().unwrap();
 
     env::remove_var("ALL_PROXY");
     env::remove_var("HTTP_PROXY");
@@ -233,7 +233,7 @@ where
 
     let result = std::panic::catch_unwind(test);
 
-    // teardown if ever needed
+    drop(guard);
 
     if let Err(ctx) = result {
         std::panic::resume_unwind(ctx);

in addition to what is proposed here. But then again, this is really only necessary if one of the tests is supposed to unwind, i.e. would only be helpful during development.

[tottoto]

Yes, and more precisely, manual dropping is only necessary when the test fails, so it seems better to move it just before resume_unwind.

[adamreichold]

Personally, I would rather suggest to make the whole complexity go away. The dependency tree for the tests already contains parking_lot. parking_lot::Mutex does not include poisoning by default (and it does not add anything for a () value being protected) and is const-constructible so that we do not need lazy_static nor OnceLock either, i.e.

diff --git a/Cargo.toml b/Cargo.toml
index ab7914f..27359a7 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -39,6 +39,7 @@ http02 = {package = "http", version = "0.2"}
 hyper = "0.14.20"
 lazy_static = "1.4.0"
 multipart = {version = "0.18.0", default-features = false, features = ["server"]}
+parking_lot = "0.12"
 rustls-pemfile = "2"
 tokio = {version = "1.20.1", features = ["full"]}
 tokio-rustls = "0.25.0"
diff --git a/src/request/proxy.rs b/src/request/proxy.rs
index d2c81d8..ae24389 100644
--- a/src/request/proxy.rs
+++ b/src/request/proxy.rs
@@ -218,26 +218,18 @@ fn with_reset_proxy_vars<T>(test: T)
 where
     T: FnOnce() + std::panic::UnwindSafe,
 {
-    use std::sync::Mutex;
+    use parking_lot::{Mutex, const_mutex};
 
-    lazy_static::lazy_static! {
-        static ref LOCK: Mutex<()> = Mutex::new(());
-    };
+    static LOCK: Mutex<()> = const_mutex(());
 
-    let _guard = LOCK.lock().unwrap();
+    let _guard = LOCK.lock();
 
     env::remove_var("ALL_PROXY");
     env::remove_var("HTTP_PROXY");
     env::remove_var("HTTPS_PROXY");
     env::remove_var("NO_PROXY");
 
-    let result = std::panic::catch_unwind(test);
-
-    // teardown if ever needed
-
-    if let Err(ctx) = result {
-        std::panic::resume_unwind(ctx);
-    }
+    test();
 }
 
 #[test]

[tottoto]

Thanks for another solution. Using parking_lot sounds good as well. Although in my opinion the mechanism which is implemented does not sound too complicated to exclude by using another crate directly, I think parking_lot is a good approach regarding simpler test implementations. I think the three options (lazy_static, OnceLock, parking_lot) might have different pros respectively, so it depends on the maintainer.