Skip to content

Commit

Permalink
Fix Key interning race. (pantsbuild#12152)
Browse files Browse the repository at this point in the history
Previously a `Key` could be observed before it was added to the
`reverse_keys` mapping leading to a panic in `key_get`. That panic used
a debug format on the problematic key and the `Debug` impl for `Key`
indirectly uses `key_get`. This results in a panic while handling a
panic.

Fix the root cause of the panic, ensuring the panic can never happen
as was intended. Also fix the recursive use of `key_get` in the panic
for sanity sake.

Fixes pantsbuild#11926

(cherry picked from commit a59be85)

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
  • Loading branch information
jsirois committed Jun 1, 2021
1 parent 38b2dd8 commit 2ee6fec
Showing 1 changed file with 18 additions and 14 deletions.
32 changes: 18 additions & 14 deletions src/rust/engine/src/interning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ use crate::externs;
pub struct Interns {
forward_keys: Mutex<HashMap<InternKey, Key, Fnv>>,
reverse_keys: RwLock<HashMap<Key, Value, Fnv>>,
// TODO(John Sirois): A volatile is all we need here since id_generator is always accessed under
// the forward_keys lock. Once the Rust memory model becomes defined, we might not even need that.
id_generator: atomic::AtomicU64,
}

Expand All @@ -58,20 +60,16 @@ impl Interns {
};

py.allow_threads(|| {
let (key, key_was_new) = {
let mut forward_keys = self.forward_keys.lock();
if let Some(key) = forward_keys.get(&intern_key) {
(*key, false)
} else {
let id = self.id_generator.fetch_add(1, atomic::Ordering::SeqCst);
let key = Key::new(id, type_id);
forward_keys.insert(intern_key, key);
(key, true)
}
};
if key_was_new {
let mut forward_keys = self.forward_keys.lock();
let key = if let Some(key) = forward_keys.get(&intern_key) {
*key
} else {
let id = self.id_generator.fetch_add(1, atomic::Ordering::SeqCst);
let key = Key::new(id, type_id);
self.reverse_keys.write().insert(key, v);
}
forward_keys.insert(intern_key, key);
key
};
Ok(key)
})
}
Expand All @@ -84,7 +82,13 @@ impl Interns {
.read()
.get(&k)
.cloned()
.unwrap_or_else(|| panic!("Previously memoized object disappeared for {:?}", k))
.unwrap_or_else(|| {
panic!(
"Previously memoized object disappeared for Key {{ id: {}, type_id: {} }}!",
k.id(),
k.type_id()
)
})
}
}

Expand Down

0 comments on commit 2ee6fec

Please sign in to comment.