Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework asynchronous asset registry #391

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

Rework asynchronous asset registry #391

wants to merge 29 commits into from

Conversation

Helco
Copy link
Owner

@Helco Helco commented Oct 30, 2024 •
edited
Loading

Some time ago I theorized that the asset registry has a race condition regarding the order of apply actions (see #344). Also during development of an asset validation command I discovered a very real deadlock (see #386) which can happen when secondary assets fail to load and some (in hindsight) shoddy locking mechanisms in Asset.PrivateLoad

I now want to:

  • Add sufficient amount of unit tests before reworking
  • Rework locking mechanism of Asset.PrivateLoad
  • Add proper asynchronous loading of assets (for parallel tools) alongside the synchronous interface (for gameplay)
  • Prevent any synchronous waits outside of the main thread
  • (at least attempt to) Prevent any asynchronous waits without cancellation tokens registered
  • Workaround STA not being available on Linux (causing NUnit to not be single-threaded)
  • Fix smaller bugs and refactorings when convenient, e.g.:
    • AssetHandle equality
    • AssetHandle.Registry in regards to local registries and handle scopes
    • Inspecting all exceptions from secondary asset loads or multiple primary asset loads
    • Prevent recursive asset dependencies

@Helco
Fix pedantic IDE warnings in asset registry
f1572e7
@Helco
Add first two test cases
5920c65
@Helco
Fix AssetHandle equality and add more test cases
a576120
@Helco
Add even more test cases
873ff60
@Helco
You guessed it.
07db850 
More test cases (with two currently failing)
@Helco
Fix WaitAsyncAll not checking disposed handles
c0223d8
@Helco
Add TestCases for applying async assets
4429778
@Helco
Add first batch of secondary asset tests
04dec31
@Helco
Couple more test cases
9734c81
@Helco
More secondary asset tests
ec63f59
@Helco
Fix apply action not called when Load while loaded asynchronously
8fe1d3a
@Helco
Improve tests for async apply
48ab189
@Helco
Use TryWrite instead of WriteAsync and lock assets on registry disposal
67a4f8c
@Helco
Replace synchronous wait on task with direct exception dispatch
572db4f
@Helco
Use Assert.Inconclusive instead of Ignore
2197015
@Helco
Add low-then-high/sync tests
fb61d3f
@Helco
Fix asset disposal on primary error
3a02dbe
@Helco
Add secondary error tests and fix exceptions being swallowed by Task.…
8c48d30 
…WhenAll
@Helco
Now I am become deadlock, the breaker of tests
1983f67
@Helco
Asset.Load is now synchronous.
4f7b12c 
This breaks everything eeeveen further
@Helco
Unfortunately fix all tests by adding async asset loading
333f5d2
@Helco
Let the deadlocks surface in the tests instead of crashing the test p…
bdbe383 
…rocess with a somewhat unrelated exception
@Helco
ci: Add timeout to test steps
9fa8fc0
@Helco
CI should have failed...
d6a607b
@Helco
Lost confidence in this approach. Let's try that again
990053f
@Helco
Revert "Lost confidence in this approach. Let's try that again"
ea240f8 
This reverts commit 990053f.
@Helco
Broke everything in another way with slightly simpler asset locking
0eb5588
@Helco
Remove STA apartment
6c50189
@Helco
Fix some problems
832cb66 
- Not queuing low assets
- Not dereferencing assets
- Not queuing apply actions
- Readd STA and postpone fixing Linux CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Helco