fix: fix scout deadlock

[wyfo]

Closes #180
Releasing GIL in Scout destructor prevents the deadlock.

[wyfo]

@Mallets @YuanYuYuan

[wyfo]

This fix brings the question of the hidden side effects of Drop implementations in Zenoh. Scout drop actually wait that the inner tasks is finished, by pausing the thread. This is quite a huge hidden thing I didn't expect.

[YuanYuYuan]

Hi @wyfo , this is a nice fix by yielding the GIL. I try to visualize the fix. Please correct me if I'm wrong.

sequenceDiagram
    participant C as GIL
    participant A as Python
    participant B as Rust
    participant D as Rust

    box zenoh-python
      participant A
      participant B
      participant C
    end

    box zenoh-rust
      participant D
    end

    A ->> A: scout.stop
    A ->> C: Acquire lock
    C ->> A: Return lock
    activate C
    A ->> C: Release lock  <br> (This fixes the issue)
    deactivate C
    A ->> B: _Scout::drop
    B ->> D: ScoutInner::drop

    D ->> B: PyClosure::drop
    B ->> C: Acquire lock
    C ->> B: Return lock
    activate C
    B ->> A: Closure.drop
    A ->> B: Closure.drop <br> return
    B ->> C: Release lock
    deactivate C
    B ->> D: PyClosure::drop <br> return

    D ->> B: ScoutInner::drop <br> return
    B ->> A: _Scout::drop <br> return

Loading

And my another fix in zenoh rust looks like this.

sequenceDiagram
    participant C as GIL
    participant A as Python
    participant B as Rust
    participant D as Rust

    box zenoh-python
      participant A
      participant B
      participant C
    end

    box zenoh-rust
      participant D
    end

    A ->> A: scout.stop
    A ->> C: Acquire lock
    C ->> A: Return lock
    activate C
    A ->> B: _Scout::drop
    B ->> D: ScoutInner::drop
    D ->> B: ScoutInner::drop <br> return
    B ->> A: _Scout::drop <br> return
    A ->> C: Release lock
    deactivate C

    D ->> D: Swtich to another thread

    D ->> B: PyClosure::drop
    B ->> C: Acquire lock
    C ->> B: Return lock
    activate C
    B ->> A: Closure.drop
    A ->> B: Closure.drop <br> return
    B ->> C: Release lock
    deactivate C
    B ->> D: PyClosure::drop <br> return

Loading

1. IINM, the Drop in zenoh rust is blocking isn't problematic. Termination in Rust just works fine. The deadlock is caused by another drop callback to Python and hence GIL is needed again.
2. TBH, I prefer your solution to my previous fix. Since the step "switch to another thread" is not deterministic.
3. Another fundamental solution is to separate the cleanup of two lands. Say, clean the callback on the Python side first, and drop the Scout on the Rust side. Therefore we can prevent this convoluted situation.

[wyfo]

You graphs need one correction: the GIL is not acquired by scout.stop, it is already acquired because Python is executing.

With your solution, I suspect the Python destructor could in fact not even be executed, because the main thread could terminate before the execution of the spawned thread.

Another fundamental solution is to separate the cleanup of two lands.

The issue is that we don't control the cleanup, because it's done on zenoh size, with the Python object embedded, and we don't control zenoh objects. I mean, it could be possible by storing the Python callback into some `Arc<Mutex<Option>>, and manually extract the Python object to prevent zenoh to call the Python drop callback, but it's quite a lot of complexity when releasing the GIL is enough.

[YuanYuYuan]

With your solution, I suspect the Python destructor could in fact not even be executed, because the main thread could terminate before the execution of the spawned thread.

Yes. That's what I said, "non-deterministic".

The issue is that we don't control the cleanup, because it's done on zenoh size, with the Python object embedded, and we don't control zenoh objects. I mean, it could be possible by storing the Python callback into some `Arc<Mutex>, and manually extract the Python object to prevent zenoh to call the Python drop callback, but it's quite a lot of complexity when releasing the GIL is enough.

I see. Then we might need to perform all the similar drop with this trick in zenoh-python.

[wyfo]

I see. Then we might need to perform all the similar drop with this trick in zenoh-python.

You want me to update the drop of the other classes?

[YuanYuYuan]

I see. Then we might need to perform all the similar drop with this trick in zenoh-python.

You want me to update the drop of the other classes?

Let's launch a discussion with the others first in the channel. I think others may want to learn more details about this problem.

To me, this PR has already been good to merge.