Use try_lock in diff_handle for Diff gutter
#11092
Conversation
- Add the method `try_load() -> Option<Diff>` to `DiffHandle`, using `try_lock` to avoid deadlocks. - Use said method in `gutter.rs diff()`, which will use a blank `GutterFn` instead when met with a locked `Diff`.
There was a problem hiding this comment.
This is not the right solution. This will make any diff gutters but the first one blank and will also randomly fail to render the diff gutter if the mutex is locked from the diff worker.
A proper fix would be using an RW lock or a change to the diff infrastructure so resource acquisition can be separate from the rendering closure.
I consider this issue really niche and easy to work around (just don't use multiple diff gutters) so I didn't prioritize fixing this
|
@pascalkuthe Appreciate the review, I hadn't considered the diff worker. I was mostly trying to avoid the complete deadlock, but this is probably too niche for a patch fix. If you think it's still worth working on, I'll try reworking it, otherwise I can drop the PR altogether. |
|
I guess using a (parking lot) rwlock is an ok solution for now that will add some overhead but it shouldn't be too bad |
|
Apologies for starting with the dirty solution first, but I appreciate the guidance @pascalkuthe! I'll keep on with this project to see if I can improve with this. On the bright side, now we can do this monstrosity!
|
* Use `try_lock` in `diff_handle` for Diff gutter - Add the method `try_load() -> Option<Diff>` to `DiffHandle`, using `try_lock` to avoid deadlocks. - Use said method in `gutter.rs diff()`, which will use a blank `GutterFn` instead when met with a locked `Diff`. * Revert changes * Replace `Mutex` with `RwLock` in `Diff` --------- Co-authored-by: Kaniel Kirby <[email protected]>
* Use `try_lock` in `diff_handle` for Diff gutter - Add the method `try_load() -> Option<Diff>` to `DiffHandle`, using `try_lock` to avoid deadlocks. - Use said method in `gutter.rs diff()`, which will use a blank `GutterFn` instead when met with a locked `Diff`. * Revert changes * Replace `Mutex` with `RwLock` in `Diff` --------- Co-authored-by: Kaniel Kirby <[email protected]>
* Use `try_lock` in `diff_handle` for Diff gutter - Add the method `try_load() -> Option<Diff>` to `DiffHandle`, using `try_lock` to avoid deadlocks. - Use said method in `gutter.rs diff()`, which will use a blank `GutterFn` instead when met with a locked `Diff`. * Revert changes * Replace `Mutex` with `RwLock` in `Diff` --------- Co-authored-by: Kaniel Kirby <[email protected]>
* Use `try_lock` in `diff_handle` for Diff gutter - Add the method `try_load() -> Option<Diff>` to `DiffHandle`, using `try_lock` to avoid deadlocks. - Use said method in `gutter.rs diff()`, which will use a blank `GutterFn` instead when met with a locked `Diff`. * Revert changes * Replace `Mutex` with `RwLock` in `Diff` --------- Co-authored-by: Kaniel Kirby <[email protected]>
The situation currently is that
render_gutterbuilds out aVecofLineDecorations, whereLineDecorationis a closure. These don't get evaluated until actually rendering, so theMutexGuardindiff_handle.load()doesn't go out of scope before being accessed again.This PR does the following:
try_load() -> Option<Diff>toDiffHandle, usingtry_lockto avoid deadlocks.gutter.rsdiff(), which will use a blankGutterFninstead when met with a lockedDiff.Fixes #11027.