cloudflare / cloudflare/pingora

Potential memory leakage issues in ConnectionPool

Open
#212 1 comment 0 reactions 1 assignee Claimed by @eaufavor View on GitHub
enhancement
Dominant language
Rust
Stars
27.4k
Forks
1.7k
Avg merge
6h 22m
Merged PRs (30d)
3

Description

## Describe the bug
- Based on the current implementation, we can find that Lru is encapsulated in `RwLock>>`. Please see https://github.com/cloudflare/pingora/blob/845c30d0825e7faf5e13ea88a9cc4949082bf00c/pingora-pool/src/lru.rs#L42-L50
- There may be the following behaviors:
- Thread 1 puts the Connection into its thread local Lru and the global `PoolNode`.
- Thread2 is responsible for executing the `idle_poll` async task(such as the `release_http_session` method in `v2.rs`). Please see https://github.com/cloudflare/pingora/blob/845c30d0825e7faf5e13ea88a9cc4949082bf00c/pingora-core/src/connectors/mod.rs#L240-L243 and https://github.com/cloudflare/pingora/blob/845c30d0825e7faf5e13ea88a9cc4949082bf00c/pingora-pool/src/connection.rs#L218-L222
- By executing the `pop_closed` method, it will successfully remove the Connection from the `PoolNode`, but it cannot remove it from the `Lru`.

## Steps to reproduce

The existence of this memory leak can be proven through the following tests:

```rust
// pingora-pool/src/connection.rs
#[test]
fn test_pop_multi_threads() {
use env_logger;

let _ = env_logger::builder()
.is_test(true)
.filter_level(log::LevelFilter::Debug)
.try_init();

let meta = ConnectionMeta::new(101, 1);
let value = "v1".to_string();

let cp: Arc> = Arc::new(ConnectionPool::new(3));
// put meta in main thread
cp.put(&meta, value);

{
let cp = cp.clone();
let meta = meta.clone();
std::thread::spawn(move || {
// pop meta in child thread
cp.pop_closed(&meta);
})
.join()
.unwrap();
}
}

// Add log print in the method pop_closed
fn pop_closed(&self, meta: &ConnectionMeta) {
// NOTE: which of these should be done first?
self.pop_evicted(meta);
let r = self.lru.pop(&meta.id);
debug!("pop from lru res: {}",r.is_some()); // the added log print
}
```
Console printing:
```
running 1 test
[2024-04-20T02:51:48Z DEBUG pingora_pool::connection] evict fd: 1 from key 101
[2024-04-20T02:51:48Z DEBUG pingora_pool::connection] pop from lru res: false
test connection::test_pop_multi_threads ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.00s
```
Currently, the maximum size of memory leaks depends on the size setting of the ConnectionPool.

If I haven't missed any other important information, I guess the `RwLock>` is mainly to reduce lock contention. Another solution to this is to use the sharded lock way(every connection has its ID, which should not be difficult to achieve).

There are use cases in the Rocksdb project on how to use sharded cache, please refer to: https://github.com/facebook/rocksdb/blob/master/cache/sharded_cache.cc

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.