Skip to content

Conversation

@egil
Copy link
Member

@egil egil commented May 19, 2022

Fixed #577.

THIS is experimental.

My idea was to, instead of using a lock around the the render tree getting build and find component's traversing the render tree to find components, is to just run the find component logic inside the dispatcher for the renderer. That should ensure that only at the time is allowed to happen.

Pull request description

PR meta checklist

  • Pull request is targeted at main branch for code
    or targeted at stable branch for documentation that is live on bunit.dev.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Code PR specific checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

@egil egil requested a review from linkdotnet May 19, 2022 23:17
@egil egil force-pushed the experiments/deadlock-findcomponent-waitfor branch from 8c5f74e to 8ffda48 Compare May 19, 2022 23:26
@egil
Copy link
Member Author

egil commented May 20, 2022

I need to pressure test this more. But after sleeping on this I am more confident in the solution than yesterday ☺️

@linkdotnet
Copy link
Collaborator

Just to recap why this is a better solution than the lock:

  • We enter FindComponents from the dispatcher (let's call it UI-thread)
  • Now if we have async code we return to the test-runner
  • In case of WaitForXXX the magic happens: As this gets triggered via the UI thread we have blocking behavior and don't need the lock.

@linkdotnet
Copy link
Collaborator

I guess to some degree also the lock in WaitForXXX can be deleted.
Especially when the initial OnRender check is also done by the UI thread. We just have to enforce that

@egil
Copy link
Member Author

egil commented May 20, 2022

I guess to some degree also the lock in WaitForXXX can be deleted. Especially when the initial OnRender check is also done by the UI thread. We just have to enforce that

I have been testing the changes with the code RaphaelMarcouxCTRL provided, and did run into another deadlock that was caused by the call to OnAfterRender(this, eventArgs.Empty); after the subscription to renderedFragment.OnAfterRender. With it stil their, his test failed almost instantly, without, I just ran it 100 times in a loop without issue.

That said, I still think the lock in WaitForHelper is needed, since there is a timer that runs in a different thread that could trigger a clean up/dispose of the WaitForHelper, at the same time as a OnAfterRender check is being processed.

@egil
Copy link
Member Author

egil commented May 20, 2022

OK, so the latest fix does cause other tests to fail consistently. Ill keep experimenting 🤕

@egil
Copy link
Member Author

egil commented May 20, 2022

Just to recap why this is a better solution than the lock:

  • We enter FindComponents from the dispatcher (let's call it UI-thread)
  • Now if we have async code we return to the test-runner

Not really, since FindComponents are synchronous all the way through, meaning that it will complete without ever being async.

It's just a way for us to schedule the FindComponents call on the same scheduler as the renderer. If you look at Renderer.cs you will see they use a similar trick in there as well.

  • In case of WaitForXXX the magic happens: As this gets triggered via the UI thread we have blocking behavior and don't need the lock.

There is a race condition between the timeout timer and the OnAfterRender updates. But a lock might be too heavy and something else could be used to say "don't dispose while there is an OnAfterRender check in progress, and don't start a OnAfterRender check after dispose has started".

@egil egil force-pushed the experiments/deadlock-findcomponent-waitfor branch from 85cb899 to b0c300d Compare May 20, 2022 14:48
@egil egil force-pushed the experiments/deadlock-findcomponent-waitfor branch from 878ce64 to 67dc158 Compare May 20, 2022 14:55
@egil
Copy link
Member Author

egil commented May 20, 2022

Ps. I will rewrite the commit when things looks like they are working

@egil egil force-pushed the experiments/deadlock-findcomponent-waitfor branch 2 times, most recently from 75a7d46 to f1fbc88 Compare May 20, 2022 17:26
@egil egil marked this pull request as ready for review May 20, 2022 17:27
@egil egil requested a review from linkdotnet May 20, 2022 17:27
@egil
Copy link
Member Author

egil commented May 20, 2022

@linkdotnet I think this is it. Let me know if you can spot anything wrong with this.

I could not figure out a way to actually test that the renderer is blocked while FindComponents is doing its thing. Did test it manually by having a component constantly re-render and adding a thread.sleep to FindComponents, and can see that without forcing FindComponents to run in the same context as the renderer, the constant rerendering component doesnt stop.

@linkdotnet
Copy link
Collaborator

Good work on this one. Especially the WaitForHelper is IMHO more readable.

@egil egil force-pushed the experiments/deadlock-findcomponent-waitfor branch from f1fbc88 to 856b9a0 Compare May 21, 2022 10:12
@egil egil requested a review from linkdotnet May 21, 2022 10:42
linkdotnet
linkdotnet previously approved these changes May 21, 2022
@egil egil force-pushed the experiments/deadlock-findcomponent-waitfor branch from bb5689f to e3c1902 Compare May 21, 2022 11:32
fixes #577.

The problem is that FindComponents traverses down the render tree when invoked, and this ensures that
no renders happens while it does so, without using locks like previous, which could result in deadlocks.

fix: aways wrap FindComponentsInternal in Dispatcher

fix: optimize wait for logging

fix: ensure failure tasks in WaitForHelper run on Renderer schedular
@egil egil force-pushed the experiments/deadlock-findcomponent-waitfor branch from e3c1902 to 7bb24ac Compare May 21, 2022 11:35
@egil egil merged commit bf35912 into main May 21, 2022
@egil egil deleted the experiments/deadlock-findcomponent-waitfor branch May 21, 2022 11:36
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.

Deadlock when using FindComponent(s) with WaitForX

3 participants