GdCell::borrow_mut should block on main thread if shared ref exists

[TitanNano]

The main thread sometimes panics if a shared borrow to a GdCell on another thread exists while trying to mutably borrow the cell.

This is caused by the ThreadTracker::mut_thread ThreadId being set to the current thread when creating the GdCell. The thread then assumes to already hold a mutable borrow and does not block.

A solution to this problem is to move the claim to the mutable borrow to which ever thread is creating a shared borrow first.

Alternatives

An alternative would be to also introduce a mutable reference count per thread. This would be clearer but introduce some additional complexity.

Just making ThreadTracker::mut_thread optional does not work due to the reentrant nature of the cell. Without a clear reference count, it's not possible to tell when the mut_thread should be cleared. (There could be inaccessible mutable borrows which bind the mutable reference to their thread.)

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-787

[Bromeon]

An alternative would be to also introduce a mutable reference count per thread. This would be clearer but introduce some additional complexity.

The current semantics are a bit difficult to understand indeed. Maybe it's because a separate shared/mut count would be clearer, or maybe it just lacks some comments 🤔 I made a corresponding remark in the code, but maybe wait with addressing changes just yet.

Any opinion on this, @lilizoey?

[Ughuuu]

Would this only happen for multithreaded use case though? Can we force that it is only for those use cases?
In single threaded case I would still like to have panic and not infinite block/hang in case of double borrow.

[TitanNano]

Would this only happen for multithreaded use case though? Can we force that it is only for those use cases? In single threaded case I would still like to have panic and not infinite block/hang in case of double borrow.

This is a bug fix for an existing feature in the "experimental-threads" crate feature. Regardless if "experimental-threads" is enabled or not, there should never be an infinite hang.
The blocking variant of GdCell has been built to coordinate borrows between multiple threads while retaining the panic behavior inside individual threads.

[Bromeon]

@Ughuuu see #736

[Bromeon]

@TitanNano Did you have a chance to look into the suggestions yet? 🙂

Maybe also cc @lilizoey for review 🔭

[TitanNano]

@TitanNano Did you have a chance to look into the suggestions yet? 🙂

Maybe also cc @lilizoey for review 🔭

Not yet.

I made a corresponding remark in the code, but maybe wait with addressing changes just yet.

I held off on changes due to this remark and went into other topics while waiting for feedback from @lilizoey. But I will look into addressing the feedback.

[Bromeon]

True, forgot about that -- it's fine for me either way 🙂

[lilizoey]

So is the goal here for the logic to be that when attempting to borrow:

if there are no conflicting borrows (i.e mut conflicts with all, shared only conflicts with mut) then perform the borrow.

otherwise, if there is a conflicting borrow on the same thread then panic

finally, if all conflicting borrows are on different threads, then block

[TitanNano]

if there are no conflicting borrows (i.e mut conflicts with all, shared only conflicts with mut) then perform the borrow.

otherwise, if there is a conflicting borrow on the same thread then panic

finally, if all conflicting borrows are on different threads, then block

Yes, that is accurate.

[lilizoey]

One thing i dont quite get is why ThreadTracker::mut_thread is just a ThreadId and not like an Option<ThreadId>? To me, the most obvious semantics of this would be "mut_thread is set to the current thread that has the mutable borrow, and unset when that thread no longer has the mutable borrow". But the current semantics are a bit weird to me?

Would changing it to an Option<ThreadId> also fix this issue? Since then it can be initialized to None.

[TitanNano]

Would changing it to an Option also fix this issue? Since then it can be initialized to None.

@lilizoey I tired to describe this here:

Just making ThreadTracker::mut_thread optional does not work due to the reentrant nature of the cell. Without a clear reference count, it's not possible to tell when the mut_thread should be cleared. (There could be inaccessible mutable borrows which bind the mutable reference to their thread.)

---

unset when that thread no longer has the mutable borrow

"No longer has to borrow" has to be tracked in drop of MutGuardBlocking, but since we have inaccessible mut guards as well, this cannot simply set the ThreadId to None every time a mut guard is dropped. We have to be sure that all MutGuardBlocking in a thread have been dropped before clearing the ThreadID.

The current implementation avoids this additional overhead by always assigning a ThreadId after successfully acquiring a borrow (either a mutable borrow or now also the first immutable borrow). This has the least amount of overhead but makes the semantics of ThreadTracker::mut_thread more complex to follow.

[lilizoey]

oh right it's the same logic as in gdcell for tracking the previous pointer, so you'd need to maintain the stacked nature of the mutable thread ids in the same way.

[Bromeon]

Thanks a lot!