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.

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

Fix: null-reference on double disposal #2245

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

NickKhalow
Copy link
Collaborator

What does this PR change?

Added OnDispose safe method that guarantees it will not be called more than once. If such attempt occurs it will be logged and propagated to Sentry

How to test the changes?

Follow ticket: #2209

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

@NickKhalow NickKhalow self-assigned this Oct 1, 2024
@NickKhalow NickKhalow linked an issue Oct 1, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Oct 1, 2024

badge

Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below.

Name Link
Commit a613fb2
Logs https://github.com/decentraland/unity-explorer/actions/runs/11166482558
Download Windows https://github.com/decentraland/unity-explorer/suites/29178917741/artifacts/2012233645
Download Mac https://github.com/decentraland/unity-explorer/suites/29178917741/artifacts/2012196146
Built on 2024-10-03T17:47:59Z

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM 💪

Copy link
Collaborator

@mikhail-dcl mikhail-dcl left a comment

Choose a reason for hiding this comment

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

I have doubts about this PR:

  • Dispose is called from a single point, if it is called multiple time all systems will spam with the error, we don't call Dispose per system, we call dispose on the entire world

  • I believe the source of the issue was diagnosed improperly:

    • Initialize is called when the whole scene environment is constructed
    • When the loading is cancelled in the middle (this is what you see when the realm/scene is frequently loaded/changed/unloaded) SceneInstanceDependencies is disposed of before Initialize is called, it's by design: take a look at SceneFactory:
              if (ct.IsCancellationRequested)
              {
                  await UniTask.SwitchToMainThread(PlayerLoopTiming.Initialization);
                  deps.Dispose();
                  sceneRuntime?.Dispose();
                  throw new OperationCanceledException();
              }
    

So the only thing that should be done is nullity checks of raycasts results because they could be not initialized (and it's the correct flow). I propose to close this PR

{
if (isDisposed)
{
ReportHub.LogError(ReportCategory.ENGINE, $"Attempt to dispose already disposed system: {this.GetType().Name}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use GetReportData instead to get the proper coordinates and log category

@NickKhalow
Copy link
Collaborator Author

@mikhail-dcl
The corresponding errors are gone for me:
#2272
#2209

So if the cause is defined improperly I think it's good to have this safety net since Dispose() is not supposed to be called multiple times in any way.

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.

Null Reference in Raycast on world switching
3 participants