-
-
Notifications
You must be signed in to change notification settings - Fork 225
Implement process and instance isolation #4498
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
Open
jamescrosswell
wants to merge
25
commits into
main
Choose a base branch
from
isolated-cache
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
83a3c75
Implement process and instance isolation
jamescrosswell 5e2c12e
Experimenting with named mutexes and semaphores
jamescrosswell 7382cac
Format code
getsentry-bot ec60ca1
Update CachingTransport.cs
jamescrosswell dcec5e0
Update CacheDirectoryCoordinator.cs
jamescrosswell 220884b
Switched to a file lock
jamescrosswell 180ff53
Update Sentry.csproj
jamescrosswell 03c998c
Format code
getsentry-bot 71ed31b
Fixed tests
jamescrosswell d5dc55a
Update CachingTransportTests.cs
jamescrosswell 3ae4d96
Merge branch 'isolated-cache' of https://github.com/getsentry/sentry-…
jamescrosswell 0046933
Update CHANGELOG.md
jamescrosswell d6dd817
.
jamescrosswell 90ede6b
Fixed mobile tests
jamescrosswell c26c5bd
Format code
getsentry-bot 39be526
Windows tests
getsentry-bot c85161f
Create CacheDirectoryCoordinatorTests.cs
jamescrosswell 536d9e2
Update SentryOptionsTests.cs
jamescrosswell 477361b
Merge branch 'isolated-cache' of https://github.com/getsentry/sentry-…
jamescrosswell ee30f23
Added tests for SalvageAbandonedCacheSessions
jamescrosswell cd0848d
Update CacheDirectoryCoordinatorTests.cs
jamescrosswell b763698
Merge branch 'main' into isolated-cache
jamescrosswell a7b0656
Update CHANGELOG.md
jamescrosswell 78f1a3a
Merge branch 'main' into isolated-cache
jamescrosswell 29fa252
Update CHANGELOG.md
jamescrosswell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,153 @@ | ||
using Sentry.Extensibility; | ||
using Sentry.Internal.Extensions; | ||
|
||
namespace Sentry.Internal; | ||
|
||
internal class CacheDirectoryCoordinator : IDisposable | ||
{ | ||
private readonly IDiagnosticLogger? _logger; | ||
private readonly IFileSystem _fileSystem; | ||
private readonly object _gate = new(); | ||
|
||
private Stream? _lockStream; | ||
private readonly string _lockFilePath; | ||
|
||
private bool _acquired; | ||
private bool _disposed; | ||
|
||
public CacheDirectoryCoordinator(string cacheDir, IDiagnosticLogger? logger, IFileSystem fileSystem) | ||
{ | ||
_logger = logger; | ||
_fileSystem = fileSystem; | ||
_lockFilePath = $"{cacheDir}.lock"; | ||
|
||
try | ||
{ | ||
var baseDir = Path.GetDirectoryName(_lockFilePath); | ||
if (!string.IsNullOrWhiteSpace(baseDir)) | ||
{ | ||
// Not normally necessary, but just in case | ||
fileSystem.CreateDirectory(baseDir); | ||
} | ||
} | ||
catch (Exception ex) | ||
{ | ||
_logger?.LogError("Failed to ensure lock directory exists for cache coordinator.", ex); | ||
} | ||
} | ||
jamescrosswell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public bool TryAcquire() | ||
{ | ||
if (_acquired) | ||
{ | ||
return true; | ||
} | ||
|
||
lock (_gate) | ||
{ | ||
if (_acquired) | ||
{ | ||
return true; | ||
} | ||
|
||
if (_disposed) | ||
{ | ||
return false; | ||
} | ||
|
||
try | ||
{ | ||
_acquired = _fileSystem.TryCreateLockFile(_lockFilePath, out _lockStream); | ||
return _acquired; | ||
} | ||
catch (Exception ex) | ||
{ | ||
_logger?.LogDebug("Unable to acquire cache directory lock", ex); | ||
} | ||
finally | ||
{ | ||
if (!_acquired && _lockStream is not null) | ||
{ | ||
try | ||
{ _lockStream.Dispose(); } | ||
catch | ||
{ | ||
// Ignore | ||
} | ||
_lockStream = null; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
lock (_gate) | ||
{ | ||
if (_disposed) | ||
{ | ||
return; | ||
} | ||
|
||
_disposed = true; | ||
|
||
if (_acquired) | ||
{ | ||
try | ||
{ | ||
_lockStream?.Close(); | ||
} | ||
catch (Exception ex) | ||
{ | ||
_logger?.LogError("Error releasing the cache directory file lock.", ex); | ||
} | ||
} | ||
|
||
try | ||
{ | ||
_lockStream?.Dispose(); | ||
} | ||
catch (Exception ex) | ||
{ | ||
_logger?.LogError("Error disposing cache lock stream.", ex); | ||
} | ||
_lockStream = null; | ||
} | ||
} | ||
} | ||
|
||
internal static class CacheDirectoryHelper | ||
{ | ||
public const string IsolatedCacheDirectoryPrefix = "isolated_"; | ||
|
||
internal static string? GetBaseCacheDirectoryPath(this SentryOptions options) => | ||
string.IsNullOrWhiteSpace(options.CacheDirectoryPath) | ||
? null | ||
: Path.Combine(options.CacheDirectoryPath, "Sentry"); | ||
|
||
internal static string? GetIsolatedFolderName(this SentryOptions options) | ||
{ | ||
var stringBuilder = new StringBuilder(IsolatedCacheDirectoryPrefix); | ||
#if IOS || ANDROID | ||
// On iOS or Android the app is already sandboxed, so there's no risk of sending data to another Sentry's DSN. | ||
// However, users may still initiate the SDK multiple times within the process, so we need an InitCounter | ||
stringBuilder.Append(options.InitCounter.Count); | ||
#else | ||
if (string.IsNullOrWhiteSpace(options.Dsn)) | ||
{ | ||
return null; | ||
} | ||
var processId = options.ProcessIdResolver.Invoke() ?? 0; | ||
stringBuilder.AppendJoin('_', options.Dsn.GetHashString(), processId, options.InitCounter.Count); | ||
#endif | ||
return stringBuilder.ToString(); | ||
} | ||
|
||
internal static string? TryGetIsolatedCacheDirectoryPath(this SentryOptions options) => | ||
GetBaseCacheDirectoryPath(options) is not { } baseCacheDir | ||
|| GetIsolatedFolderName(options) is not { } isolatedFolderName | ||
? null | ||
: Path.Combine(baseCacheDir, isolatedFolderName); | ||
} |
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The double-checked locking pattern is correctly implemented, but the _acquired field should be marked as volatile to ensure proper memory visibility across threads. This prevents potential race conditions where one thread might not see the updated value immediately.
Did we get this right? 👍 / 👎 to inform future reviews.