Skip to content

Fixes #2073. Fixes regression introduced in v1.8.2: Nesting MainLoop.Invoke deadlocks#2083

Merged
tig merged 1 commit intogui-cs:developfrom
tznind:fix-deadlock
Oct 20, 2022
Merged

Fixes #2073. Fixes regression introduced in v1.8.2: Nesting MainLoop.Invoke deadlocks#2083
tig merged 1 commit intogui-cs:developfrom
tznind:fix-deadlock

Conversation

@tznind
Copy link
Copy Markdown
Collaborator

@tznind tznind commented Oct 13, 2022

Fixes #2073 - Callback method execution RunIdle() no longer runs inside a lock on idleHandlersLock

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tznind
Copy link
Copy Markdown
Collaborator Author

tznind commented Oct 13, 2022

@BDisp sorry that I have been a bit out of the loop after reporting this (have been on half term holidays).

I dug into the locking situation and found this issue. RunIdle has its own locking logic and seems to take pains not to lock during callback execution (which is good). But before this PR the whole RunIdle was already wrapped in the lock by MainIteration.

This PR removes that logic. It seems to fix #2073 and restore old behaviour.

Let me know if this is not the right fix. I'm not sure how this impacts your other changes in invoke. Let me know.

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 13, 2022

Thanks @tznind, before Tuesday I can't, holydays too. But I think your PR fix it since it run the idle outside the lock.

@tig
Copy link
Copy Markdown
Member

tig commented Oct 13, 2022

I'm confused by the comments above. Do both of you agree this PR should be merged now?

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 13, 2022

I'm confused by the comments above. Do both of you agree this PR should be merged now?

Perhaps yes, but I can't test it before Tuesday.

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Oct 20, 2022

I confirm that this PR fix the #2073 issue. I already closed my PR #2075 for this to be accept. I only submitted the PR tznind#133 with a unit test for this issue won't happen again.

@tig tig changed the title Fixes #2073 Fix locking for the entire RunIdle callback execution Fixes #2073. Fix locking for the entire RunIdle callback execution Oct 20, 2022
@tig tig changed the title Fixes #2073. Fix locking for the entire RunIdle callback execution Fixes #2073. Fixes regression introduced in v1.8.2: Nesting MainLoop.Invoke deadlocks Oct 20, 2022
Copy link
Copy Markdown
Member

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @tznind & @BDisp!

@tig tig merged commit 1357295 into gui-cs:develop Oct 20, 2022
This was referenced Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can no longer nest Application.MainLoop.Invoke (since 1.8.2)

3 participants