brave/adblock-rust

`Engine` struct is no longer `core::marker::Sync`

ArniDagur opened this issue · 2 comments

The PyO3 library requires structures to be Sync. This means that python-adblock requires Engine to be Sync. Previously, this was made possible by disabling the object-pooling feature. However, there has been a regression, and Engine is unconditionally Sync.

The immediate cause is that there is a *const NetworkFilter pointer in the RegexManager structure. This can be fixed by applying the following patch:

diff --git a/src/regex_manager.rs b/src/regex_manager.rs
index 7217086..4e86c5a 100644
--- a/src/regex_manager.rs
+++ b/src/regex_manager.rs
@@ -57,7 +57,7 @@ impl Default for RegexManagerDiscardPolicy {
 type RandomState = std::hash::BuildHasherDefault<seahash::SeaHasher>;
 
 pub struct RegexManager {
-    map: HashMap<*const NetworkFilter, RegexEntry, RandomState>,
+    map: HashMap<usize, RegexEntry, RandomState>,
     compiled_regex_count: usize,
     now: Instant,
     last_cleanup: Instant,
@@ -90,7 +90,10 @@ impl RegexManager {
         if !filter.is_regex() && !filter.is_complete_regex() {
             return true;
         }
-        let key = filter as *const NetworkFilter;
+        // Converting to `usize` prevents `*const NetworkFilter` appearing in
+        // the struct definition of `RegexManager`, which keeps `RegexManager`
+        // `core::marker::Sync`.
+        let key = filter as *const NetworkFilter as usize;
         use std::collections::hash_map::Entry;
         match self.map.entry(key) {
             Entry::Occupied(mut e) => {

There may be more places that need fixing, but I haven't found out yet because of the issue described in #259.

@ArniDagur sorry about that! Thanks for pointing it out.

I think we should be able to use NetworkFilter::get_id here instead of a pointer (i.e. content-addressing the filters); let me give that a try.

cc @atuchin-m

@ArniDagur it should be fixed in v0.7.3, but please let me know if that isn't working for you!