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 exceptions never being reported when UniTask is executed without await and Forget() #323

Merged
merged 2 commits into from Mar 2, 2022

Conversation

SolidAlloy
Copy link
Contributor

@SolidAlloy SolidAlloy commented Oct 16, 2021

Fixes issue #315

I tried following your advice to replace Exception ex with ExceptionHolder. However, it does not resolve the issue because the following happens:

  1. ExceptionHolder is created and assigned to the field.
  2. Task getter is executed and UniTask.FromException(ex.GetException().SourceException) is returned.
  3. GetException() suppresses the ExceptionHolder's finalizer.
  4. The returned task is not awaited, and thus the exception is not reported again.

Instead, I found that the issue lies in another part of the code.
AsyncUniTaskMethodBuilder.Task getter is executed even if the task is not assigned to a variable:

private void Awake()
{
    TestAsync();
}

private async UniTask TestAsync()
{
    throw new Exception();
}

Thus, we can't know if the task returned by the Task getter is going to be awaited or not. We need a backup for a case when the task is not going to be awaited.

In the end, I added a finalizer to ExceptionResultSource:

~ExceptionResultSource()
{
    if (!calledGet)
    {
        UniTaskScheduler.PublishUnobservedTaskException(exception.SourceException);
    }
}

It acts the same as ExceptionHolder. If the exception is thrown, the finalizer will be suppressed. But if the task is never awaited, the finalizer will publish the exception.

@timcassell
Copy link

You can remove the flag and just call GC.SuppressFinalize.

@SolidAlloy
Copy link
Contributor Author

Yes, I understand your reasoning. I just copied the code from ExceptionHolder, along with the flag. Maybe neuecc had a reason to put it there .e.g there was a specific case when the finalizer was called regardless of SupressFinalize, so I decided not to remove it. Anyway, it won't hurt the performance

@github-actions
Copy link
Contributor

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Feb 17, 2022
@neuecc neuecc removed the stale label Feb 17, 2022
@neuecc
Copy link
Member

neuecc commented Mar 2, 2022

Ok, thanks!

@neuecc neuecc merged commit b089f74 into Cysharp:master Mar 2, 2022
pillsgood pushed a commit to pillsgood/UniTask that referenced this pull request May 8, 2023
Fix exceptions never being reported when UniTask is executed without await and Forget()
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.

None yet

3 participants