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

Auto Session Tracking should only be enabled by default on Mobile until total process isolation is supported #264

Open
bruno-garcia opened this issue Jul 19, 2021 · 5 comments
Labels
Bug Something isn't working

Comments

@bruno-garcia
Copy link
Member

bruno-garcia commented Jul 19, 2021

The session support currently expects exclusive access to the directory it caches data. This works well in sandboxed apps like on mobile but not on Desktop when multiple instances can be executed concurrently.

For this reason, until getsentry/sentry-dotnet#1067 is fixed, we can only opt-in to auto session tracking safely if we are sure there's only 1 instance of the app (mobile, or a player on desktop that only allows one instance running).

When PlayerSettings.forceSingleInstance is set we're safe to enable auto session tracking. Also if the player is Android or iOS and console.

@bruno-garcia bruno-garcia added Feature New feature or request Bug Something isn't working desktop and removed Feature New feature or request labels Jul 27, 2021
@semuserable
Copy link
Contributor

PlayerSettings.forceSingleInstance is present in UnityEditor.dll and not available in Sentry.Unity (references UnityEngine) where logic for this is executing (SentryOptionsUtility.SetDefaults).

There are 2 possible approaches to determine the platform: Application.platform or Platform dependent compilation.

Suggested way is to use Platform dependent compilation approach. But the problem is still in PlayerSettings.forceSingleInstance not available at the place of calling.

Possible implementation could look something like this

scriptableOptions.AutoSessionTracking = IsAutoSessionTracking(options.AutoSessionTracking);

private bool IsAutoSessionTracking(bool autoSessionTracking)
{
#if UNITY_IOS || UNITY_ANDROID || UNITY_PS4 || UNITY_XBOXONE
    return autoSessionTracking;
#elif UNITY_STANDALONE || UNITY_EDITOR
    return PlayerSettings.forceSingleInstance;
#else
    return autoSessionTracking;
#endif
}

@bruno-garcia
Copy link
Member Author

SentryWindow runs in the editor (Sentry.Unity.Editor), we could read PlayerSettings.forceSingleInstance there and set it to the scriptable object and rely on that value at runtime to disable auto session tracking if the Application.platform is standalone and forceSingleInstance is not true.

Or we live with this until we address getsentry/sentry-dotnet#1067

@bitsandfoxes
Copy link
Contributor

bitsandfoxes commented Sep 16, 2021

The SentryWindow would not get notified about a change to the player settings. But we could read them during pre-build and set it accordingly?

@vaind
Copy link
Collaborator

vaind commented Mar 11, 2022

As per a discord discussion:

  • this is rather important for the Windows standalone support so bumping to P1
  • until the dotnet issue is resolved, implement a named mutex so that exactly one instance of the app uses the cache dir

@vaind vaind moved this from Needs Discussion to Backlog in Mobile & Cross Platform SDK Mar 11, 2022
@vaind vaind moved this from Backlog to In Progress in Mobile & Cross Platform SDK Mar 17, 2022
@vaind vaind self-assigned this Mar 17, 2022
@vaind vaind moved this from In Progress to Blocked in Mobile & Cross Platform SDK Mar 17, 2022
@vaind vaind assigned vaind and unassigned vaind Mar 17, 2022
@vaind vaind moved this from Blocked to In Progress in Mobile & Cross Platform SDK Mar 17, 2022
vaind added a commit that referenced this issue Mar 17, 2022
@vaind vaind moved this from In Progress to Needs Review in Mobile & Cross Platform SDK Mar 17, 2022
vaind added a commit that referenced this issue Mar 18, 2022
@vaind vaind moved this from Needs Review to Blocked in Mobile & Cross Platform SDK Mar 21, 2022
@vaind
Copy link
Collaborator

vaind commented Mar 21, 2022

Updating impact, priority & status after the workaround has been implemented. The final solution is blocked by the dotnet issue.

vaind added a commit that referenced this issue Mar 21, 2022
vaind added a commit that referenced this issue Mar 22, 2022
…(desktop) (#643)

* fix: add a workaround for #264 - don't run multiple sessions at the same time (desktop)

* chore: update changelog

* chore: lockfile implementation comments

* refactor: move single-dotnet-instance logic to SentryUnity.cs

* chore: minor log changes

* Update src/Sentry.Unity/SentryUnity.cs

Co-authored-by: Bruno Garcia <[email protected]>

Co-authored-by: Bruno Garcia <[email protected]>
@vaind vaind removed their assignment Mar 22, 2022
@bitsandfoxes bitsandfoxes added this to GDX Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
Status: No status
Archived in project
Development

No branches or pull requests

4 participants