refactor: drop the storage finalizer - #92
Open
imurashka wants to merge 1 commit into
Open
Conversation
BinaryStorage holds no unmanaged resources, so the finalizer had nothing to release. It skipped the save, disposed collections that were already unreachable, and released the editor path lock by mutating a static HashSet from the finalizer thread while the main thread could be reading it from Construct. With the finalizer gone, disposing is the only path, so Dispose no longer needs the disposing flag or the two branches it guarded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #91, so this diff shows only the finalizer change. GitHub retargets it to
mainonce #91 merges.BinaryStorageholds no unmanaged resources: a dictionary, records, reactive collections, and a persistence layer whose onlyFileStreamis opened and closed inside a single method. The finalizer arrived in #33, a documentation commit, as part of laying out the textbook dispose pattern - nothing asked for it.On the finalizer thread it skipped the save, then disposed reactive collections that were already unreachable and had no finalizers of their own, and finally released the editor path lock by mutating a static
HashSet<string>with no lock while the main thread could be reading the same set fromConstruct. In player builds that last call is compiled out entirely, so the whole pass was dead work there.With the finalizer gone,
disposingis always true, soDispose(bool)folds back intoDispose().Behaviour change: a storage that is never disposed no longer has its editor path lock released by the GC, so a second
Constructon the same path will keep throwing until the domain reloads. Nothing changes for storages that are disposed, and no unsaved data was ever written by the finalizer.