Reentrant deadlock

[nmldiegues]

We're using bb8 (thanks for doing it!) in a service handling lots of traffic, with bb8 used millions of times per minute.
Today we hit a deadlock that seems to be induced by a single task/thread scenario due to reentrancy:

Thread 3 (Thread 0x7f4d193fe700 (LWP 99758)):
#0  syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
#1  0x0000563841825ea5 in parking_lot_core::thread_parker::imp::ThreadParker::futex_wait () at /cfsetup_build/vendor/parking_lot_core/src/thread_parker/linux.rs:112
#2  parking_lot_core::thread_parker::imp::{impl#0}::park () at /cfsetup_build/vendor/parking_lot_core/src/thread_parker/linux.rs:66
#3  parking_lot_core::parking_lot::park::{closure#0}<parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#0}, parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#1}, parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#2}> () at /cfsetup_build/vendor/parking_lot_core/src/parking_lot.rs:635
#4  parking_lot_core::parking_lot::with_thread_data<parking_lot_core::parking_lot::ParkResult, parking_lot_core::parking_lot::park::{closure_env#0}<parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#0}, parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#1}, parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#2}>> () at /cfsetup_build/vendor/parking_lot_core/src/parking_lot.rs:207
#5  parking_lot_core::parking_lot::park<parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#0}, parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#1}, parking_lot::raw_mutex::{impl#3}::lock_slow::{closure_env#2}> () at /cfsetup_build/vendor/parking_lot_core/src/parking_lot.rs:600
#6  parking_lot::raw_mutex::RawMutex::lock_slow () at src/raw_mutex.rs:262
#7  0x0000563841a3ef55 in parking_lot::raw_mutex::{impl#0}::lock () at vendor/parking_lot/src/raw_mutex.rs:72
#8  lock_api::mutex::Mutex::lock<parking_lot::raw_mutex::RawMutex, bb8::internals::PoolInternals<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>> () at vendor/lock_api/src/mutex.rs:214
#9  bb8::internals::{impl#4}::drop<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>> () at vendor/bb8/src/internals.rs:191
#10 0x000056384185007a in core::ptr::drop_in_place<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ptr/mod.rs:490
#11 core::ptr::drop_in_place<core::option::Option<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ptr/mod.rs:490
#12 core::ptr::drop_in_place<core::cell::UnsafeCell<core::option::Option<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ptr/mod.rs:490
#13 core::ptr::drop_in_place<futures_channel::lock::Lock<core::option::Option<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ptr/mod.rs:490
#14 core::ptr::drop_in_place<futures_channel::oneshot::Inner<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ptr/mod.rs:490
#15 alloc::sync::Arc::drop_slow<futures_channel::oneshot::Inner<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/alloc/src/sync.rs:1123
#16 0x0000563841914acc in alloc::sync::{impl#27}::drop<futures_channel::oneshot::Inner<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/alloc/src/sync.rs:1745
#17 core::ptr::drop_in_place<alloc::sync::Arc<futures_channel::oneshot::Inner<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ptr/mod.rs:490
#18 core::ptr::drop_in_place<futures_channel::oneshot::Sender<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>>> () at /rustc/fc594f15669680fa70d255faec3ca3fb507c3405/library/core/src/ptr/mod.rs:490
#19 futures_channel::oneshot::Sender::send<bb8::internals::InternalsGuard<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>>> () at vendor/futures-channel/src/oneshot.rs:343
#20 0x0000563841a251d7 in bb8::internals::PoolInternals::put<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>> () at vendor/bb8/src/internals.rs:74
#21 0x0000563841836ac4 in bb8::inner::PoolInner::put_back<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>> () at vendor/bb8/src/inner.rs:166
#22 0x0000563841b139f7 in bb8::api::{impl#10}::drop<quicksilver_client::conn_pool::QuicksilverConnManager<quicksilver_client::conn::MemcachedConn, quicksilver_client::new::{async_fn#0}::{closure_env#0}, quicksilver_client::new::{async_fn#0}::{closure#0}::{async_block_env#0}>> () at vendor/bb8/src/api.rs:364

This is using tag v0.8.0.

If you ignore the boilerplate, you'll see essentially that:

1. a connection was being returned to the pool
2. that logic acquires the internals lock
3. then it calls PoolInternals#PoolInternals
4. which creates a InternalsGuard
5. now while the InternalsGuard is in the call stack, everything gets dropped (this is possible in our case because requests are handled in a tokio runtime with a timeout per request, and timeout happened here)
6. so InternalsGuard#drop is called, which tries to acquire the internals lock <-- and voila, we hit a deadlock since the lock is not reentrant

[djc]

Thanks for digging into this! Would you be able to submit a PR to fix it?

[nmldiegues]

Thanks for digging into this! Would you be able to submit a PR to fix it?

Fixing it with parking_lot should be trivial because we can just use https://docs.rs/parking_lot/latest/parking_lot/type.ReentrantMutex.html --- see 5c1fe4a for the example patch on top of v0.8.0 (see below for why I did this)

However I see this repo very recently had this change #152 which makes that trivial fix impossible.

Any other fix is going to require a design change as to how the connection is managed throughout the code, which puts us at a crossroads here.

[djc]

Any other fix is going to require a design change as to how the connection is managed throughout the code, which puts us at a crossroads here.

What kind of design changes? Would it introduce a lot of extra complexity or just move the code around?

[drdrsh]

@nmldiegues I think I am running into the same issue and I am able to reproduce it. I tried your patch I am a getting panic

panicked at 'already borrowed: BorrowMutError', /usr/local/cargo/git/checkouts/bb8-c4fdc7c4996dd882/5c1fe4a/bb8/src/internals.rs:196

[djc]

I've submitted #186 which tries to fix this.