paritytech / parity-common

Collection of crates used in Parity projects
https://www.paritytech.io/
Apache License 2.0
289 stars 218 forks source link

[kvdb] Lifetime of `iter_from_prefix` #367

Closed yihuang closed 4 years ago

yihuang commented 4 years ago
    fn iter_from_prefix<'a>(
        &'a self,
        col: u32,
        prefix: &'a [u8],
    ) -> Box<dyn Iterator<Item = (Box<[u8]>, Box<[u8]>)> + 'a>;

The prefix use the same lifetime marker as self and return value, so the returned iterator can't live longer than prefix. I think this's not good, because prefix could be temporarily constructed. For example:

fn iter_prefix(&self, key: Key) -> impl Iterator<Item=...> {
    self.db.iter_from_prefix(SOME_COL, &key.encode())
}
ordian commented 4 years ago

This is intended. The implementations (like rocksdb) need to hold a reference to the prefix. The user is expected to store the prefix somewhere that outlives the iterator. You can create an API that returns something like

struct Iter<'a> {
    encoded_prefix: Vec<u8>,
    db: &Db,
}
impl<'a> Iter<'a> {
    fn iter(&self) -> impl Iterator<Item=...> {
        self.db.iter_from_prefix(SOME_COL, &self.encoded_prefix)
    }
}

We could clone the prefix somewhere in the internals of prefix iterator, but it's a question of convenience vs explicitness.

ordian commented 4 years ago

The implementations (like rocksdb) need to hold a reference to the prefix.

I might be wrong on this one, let me double check.

ordian commented 4 years ago

It seems RocksDB impl doesn't require that, it uses seek(prefix) only on iterator construction [1], but our InMemory implementation of kvdb currently does. I'll close the issue since there is a workaround.