search

containerd / rust-extensions

Rust crates to extend containerd
https://containerd.io
Apache License 2.0
184 stars 73 forks source link

monitor_notify_by_pid and monitor_unsubscribe may cause dead lock #270

Closed zmrush closed 5 months ago

zmrush commented 6 months ago

monitor_notify_by_pid may be blocked by send msg to channel because bounded channel is full(but monitor lock has not be freed), on the other hand,monitor_unsubscribe may block because monitor lock is holded by the monitor_notify_by_pid

zmrush commented 6 months ago

@kzys @dims @caniszczyk @tianon hi,can anyone see this commit?thanks

zmrush commented 6 months ago

@mxpv can u check this commit?

Burning1020 commented 5 months ago

Can you find the reason why the monitor lock isn't being released? I think changing it to unbounded reduces the likelihood of this issue, but it doesn't real resolve it.

zmrush commented 5 months ago

monitor_notify_by_pid method need MONITOR lock firstly, and then send msg to channel which is bounded,therefore sending msg may be blocked,this causes the MONITOR lock not released. Other methods may be like this: firstly get the lock then copy channels to a vector, then release the monitor lock, then send msg use the new built vector of channels, but it also maybe blocked because the bounded channel, of cause by this way the lock is released and there is no dead lock, but i think it also may be blocked because of the bounded channel. So i think change the bounded channel is always needed, which is implemented in the sync feature. @Burning1020

Burning1020 commented 5 months ago

That makes sense. If the receiver doesn't consume any object, unbounded channel may lead to OOM, but that's unlikely.

ningmingxiao commented 1 month ago

I create another pr to fix this issue,we don't need to use use unbounded_channel(it doesn't real solve the problem). https://github.com/containerd/rust-extensions/pull/316