diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a0e5c1bd3..90d204945f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Features + +- Implement process and instance isolation so that multiple instances of the Sentry SDK can be instantiated inside the same process when using the Caching Transport ([#4498](https://github.com/getsentry/sentry-dotnet/pull/4498)) + ### Fixes - Fail when building Blazor WASM with Profiling. We don't support profiling in Blazor WebAssembly projects. ([#4512](https://github.com/getsentry/sentry-dotnet/pull/4512)) diff --git a/src/Sentry/GlobalSessionManager.cs b/src/Sentry/GlobalSessionManager.cs index 35125817aa..9e56ae7331 100644 --- a/src/Sentry/GlobalSessionManager.cs +++ b/src/Sentry/GlobalSessionManager.cs @@ -36,7 +36,7 @@ public GlobalSessionManager( // TODO: session file should really be process-isolated, but we // don't have a proper mechanism for that right now. - _persistenceDirectoryPath = options.TryGetDsnSpecificCacheDirectoryPath(); + _persistenceDirectoryPath = options.TryGetIsolatedCacheDirectoryPath(); } // Take pause timestamp directly instead of referencing _lastPauseTimestamp to avoid diff --git a/src/Sentry/Internal/CacheDirectoryCoordinator.cs b/src/Sentry/Internal/CacheDirectoryCoordinator.cs new file mode 100644 index 0000000000..889f964f89 --- /dev/null +++ b/src/Sentry/Internal/CacheDirectoryCoordinator.cs @@ -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); + } + } + + 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); +} diff --git a/src/Sentry/Internal/FileSystemBase.cs b/src/Sentry/Internal/FileSystemBase.cs index 60b474a668..9e377d9836 100644 --- a/src/Sentry/Internal/FileSystemBase.cs +++ b/src/Sentry/Internal/FileSystemBase.cs @@ -2,6 +2,9 @@ namespace Sentry.Internal; internal abstract class FileSystemBase : IFileSystem { + public IEnumerable EnumerateDirectories(string path, string searchPattern) => + Directory.EnumerateDirectories(path, searchPattern); + public IEnumerable EnumerateFiles(string path) => Directory.EnumerateFiles(path); public IEnumerable EnumerateFiles(string path, string searchPattern) => @@ -23,6 +26,7 @@ public IEnumerable EnumerateFiles(string path, string searchPattern, Sea public abstract bool CreateDirectory(string path); public abstract bool DeleteDirectory(string path, bool recursive = false); public abstract bool CreateFileForWriting(string path, out Stream fileStream); + public abstract bool TryCreateLockFile(string path, out Stream fileStream); public abstract bool WriteAllTextToFile(string path, string contents); public abstract bool MoveFile(string sourceFileName, string destFileName, bool overwrite = false); public abstract bool DeleteFile(string path); diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index b5ed2cb34a..a61a5d3f76 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/src/Sentry/Internal/Http/CachingTransport.cs @@ -5,14 +5,15 @@ namespace Sentry.Internal.Http; /// -/// A transport that caches envelopes to disk and sends them in the background. +/// Transport that caches envelopes to disk and sends them in the background. /// /// +/// /// Note although this class has a /// method, it doesn't implement as this caused /// a dependency issue with Log4Net in some situations. -/// -/// See https://github.com/getsentry/sentry-dotnet/issues/3178 +/// +/// See https://github.com/getsentry/sentry-dotnet/issues/3178 /// internal class CachingTransport : ITransport, IDisposable { @@ -22,9 +23,11 @@ internal class CachingTransport : ITransport, IDisposable private readonly ITransport _innerTransport; private readonly SentryOptions _options; private readonly bool _failStorage; - private readonly string _isolatedCacheDirectoryPath; private readonly int _keepCount; + private readonly CacheDirectoryCoordinator _cacheCoordinator; + private readonly string _isolatedCacheDirectoryPath; + // When a file is getting processed, it's moved to a child directory // to avoid getting picked up by other threads. private readonly string _processingDirectoryPath; @@ -45,6 +48,7 @@ internal class CachingTransport : ITransport, IDisposable private readonly CancellationTokenSource _workerCts = new(); private Task _worker = null!; + private Task? _recycler = null; private ManualResetEventSlim? _initCacheResetEvent = new(); private ManualResetEventSlim? _preInitCacheResetEvent = new(); @@ -75,8 +79,13 @@ private CachingTransport(ITransport innerTransport, SentryOptions options, bool : 0; // just in case MaxCacheItems is set to an invalid value somehow (shouldn't happen) _isolatedCacheDirectoryPath = - options.TryGetProcessSpecificCacheDirectoryPath() ?? + options.TryGetIsolatedCacheDirectoryPath() ?? throw new InvalidOperationException("Cache directory or DSN is not set."); + _cacheCoordinator = new CacheDirectoryCoordinator(_isolatedCacheDirectoryPath, options.DiagnosticLogger, options.FileSystem); + if (!_cacheCoordinator.TryAcquire()) + { + throw new InvalidOperationException("Cache directory already locked."); + } // Sanity check: This should never happen in the first place. // We check for `DisableFileWrite` before creating the CachingTransport. @@ -90,15 +99,15 @@ private void Initialize(bool startWorker) // Restore any abandoned files from a previous session MoveUnprocessedFilesBackToCache(); - // Ensure directories exist - _fileSystem.CreateDirectory(_isolatedCacheDirectoryPath); _fileSystem.CreateDirectory(_processingDirectoryPath); - // Start a worker, if one is needed if (startWorker) { _options.LogDebug("Starting CachingTransport worker."); _worker = Task.Run(CachedTransportBackgroundTaskAsync); + + // Start a recycler in the background so as not to block initialisation + _recycler = Task.Run(() => SalvageAbandonedCacheSessions(_workerCts.Token), _workerCts.Token); } else { @@ -178,6 +187,48 @@ private async Task CachedTransportBackgroundTaskAsync() _options.LogDebug("CachingTransport worker stopped."); } + internal void SalvageAbandonedCacheSessions(CancellationToken cancellationToken) + { + if (_options.GetBaseCacheDirectoryPath() is not { } baseCacheDir) + { + return; + } + + const string searchPattern = $"{CacheDirectoryHelper.IsolatedCacheDirectoryPrefix}*"; + foreach (var dir in _fileSystem.EnumerateDirectories(baseCacheDir, searchPattern)) + { + if (cancellationToken.IsCancellationRequested) + { + return; // graceful exit on cancellation + } + + if (string.Equals(dir, _isolatedCacheDirectoryPath, StringComparison.OrdinalIgnoreCase)) + { + continue; // Ignore the current cache directory + } + + _options.LogDebug("found abandoned cache: {0}", dir); + using var coordinator = new CacheDirectoryCoordinator(dir, _options.DiagnosticLogger, _options.FileSystem); + if (!coordinator.TryAcquire()) + { + continue; + } + + _options.LogDebug("Salvaging abandoned cache: {0}", dir); + foreach (var filePath in _fileSystem.EnumerateFiles(dir, "*.envelope", SearchOption.AllDirectories)) + { + if (cancellationToken.IsCancellationRequested) + { + return; // graceful exit on cancellation + } + + var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath)); + _options.LogDebug("Moving abandoned file back to cache: {0} to {1}.", filePath, destinationPath); + MoveFileWithRetries(filePath, destinationPath, "abandoned"); + } + } + } + private void MoveUnprocessedFilesBackToCache() { // Processing directory may already contain some files left from previous session @@ -194,42 +245,46 @@ private void MoveUnprocessedFilesBackToCache() { var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath)); _options.LogDebug("Moving unprocessed file back to cache: {0} to {1}.", filePath, destinationPath); + MoveFileWithRetries(filePath, destinationPath, "unprocessed"); + } + } - const int maxAttempts = 3; - for (var attempt = 1; attempt <= maxAttempts; attempt++) + private void MoveFileWithRetries(string filePath, string destinationPath, string moveType) + { + const int maxAttempts = 3; + for (var attempt = 1; attempt <= maxAttempts; attempt++) + { + try { - try + _fileSystem.MoveFile(filePath, destinationPath); + break; + } + catch (Exception ex) + { + if (!_fileSystem.FileExists(filePath)) { - _fileSystem.MoveFile(filePath, destinationPath); + _options.LogDebug( + "Failed to move {2} file back to cache (attempt {0}), " + + "but the file no longer exists so it must have been handled by another process: {1}", + attempt, filePath, moveType); break; } - catch (Exception ex) - { - if (!_fileSystem.FileExists(filePath)) - { - _options.LogDebug( - "Failed to move unprocessed file back to cache (attempt {0}), " + - "but the file no longer exists so it must have been handled by another process: {1}", - attempt, filePath); - break; - } - - if (attempt < maxAttempts) - { - _options.LogDebug( - "Failed to move unprocessed file back to cache (attempt {0}, retrying.): {1}", - attempt, filePath); - Thread.Sleep(200); // give a small bit of time before retry - } - else - { - _options.LogError(ex, - "Failed to move unprocessed file back to cache (attempt {0}, done.): {1}", attempt, filePath); - } + if (attempt < maxAttempts) + { + _options.LogDebug( + "Failed to move {2} file back to cache (attempt {0}, retrying.): {1}", + attempt, filePath, moveType); - // note: we do *not* want to re-throw the exception + Thread.Sleep(200); // give a small bit of time before retry } + else + { + _options.LogError(ex, + "Failed to move {2} file back to cache (attempt {0}, done.): {1}", attempt, filePath, moveType); + } + + // note: we do *not* want to re-throw the exception } } } @@ -278,7 +333,7 @@ private async Task ProcessCacheAsync(CancellationToken cancellation) // Make sure no files got stuck in the processing directory // See https://github.com/getsentry/sentry-dotnet/pull/3438#discussion_r1672524426 - if (_options.NetworkStatusListener is { Online: false } listener) + if (_options.NetworkStatusListener is { Online: false }) { MoveUnprocessedFilesBackToCache(); } @@ -539,6 +594,20 @@ public Task StopWorkerAsync() return _worker; } + private Task StopRecyclerAsync() + { + if (_recycler?.IsCompleted != false) + { + // already stopped + return Task.CompletedTask; + } + + // Stop worker and wait until it finishes + _options.LogDebug("Stopping CachingTransport worker."); + _workerCts.Cancel(); + return _recycler; + } + public Task FlushAsync(CancellationToken cancellationToken = default) { _options.LogDebug("CachingTransport received request to flush the cache."); @@ -557,10 +626,21 @@ public async ValueTask DisposeAsync() _options.LogError(ex, "Error stopping worker during dispose."); } + try + { + await StopRecyclerAsync().ConfigureAwait(false); + } + catch (Exception ex) + { + _options.LogError(ex, "Error in recycler during dispose."); + } + _workerSignal.Dispose(); _workerCts.Dispose(); _worker.Dispose(); + _recycler?.Dispose(); _cacheDirectoryLock.Dispose(); + _cacheCoordinator.Dispose(); _preInitCacheResetEvent?.Dispose(); _initCacheResetEvent?.Dispose(); diff --git a/src/Sentry/Internal/IFileSystem.cs b/src/Sentry/Internal/IFileSystem.cs index 366bf70583..dd44d67b84 100644 --- a/src/Sentry/Internal/IFileSystem.cs +++ b/src/Sentry/Internal/IFileSystem.cs @@ -10,6 +10,7 @@ internal interface IFileSystem // Note: This is not comprehensive. If you need other filesystem methods, add to this interface, // then implement in both Sentry.Internal.FileSystem and Sentry.Testing.FakeFileSystem. + public IEnumerable EnumerateDirectories(string path, string searchPattern); public IEnumerable EnumerateFiles(string path); public IEnumerable EnumerateFiles(string path, string searchPattern); public IEnumerable EnumerateFiles(string path, string searchPattern, SearchOption searchOption); @@ -22,6 +23,7 @@ internal interface IFileSystem public bool CreateDirectory(string path); public bool DeleteDirectory(string path, bool recursive = false); public bool CreateFileForWriting(string path, out Stream fileStream); + public bool TryCreateLockFile(string path, out Stream fileStream); public bool WriteAllTextToFile(string path, string contents); public bool MoveFile(string sourceFileName, string destFileName, bool overwrite = false); public bool DeleteFile(string path); diff --git a/src/Sentry/Internal/InitCounter.cs b/src/Sentry/Internal/InitCounter.cs new file mode 100644 index 0000000000..57155a9dc8 --- /dev/null +++ b/src/Sentry/Internal/InitCounter.cs @@ -0,0 +1,23 @@ +namespace Sentry.Internal; + +/// +/// This is used internally to track the number of times the SDK has been initialised so that different +/// Hub instances get a different cache directory path, even if they're running in the same process. +/// +internal interface IInitCounter +{ + public int Count { get; } + public void Increment(); +} + +/// +internal class InitCounter : IInitCounter +{ + internal static InitCounter Instance { get; } = new(); + + private int _count; + + public int Count => Volatile.Read(ref _count); + + public void Increment() => Interlocked.Increment(ref _count); +} diff --git a/src/Sentry/Internal/Polyfills.cs b/src/Sentry/Internal/Polyfills.cs index 2113687f8d..4666a1f568 100644 --- a/src/Sentry/Internal/Polyfills.cs +++ b/src/Sentry/Internal/Polyfills.cs @@ -1,5 +1,9 @@ // Polyfills to bridge the missing APIs in older targets. +#if !NET9_0_OR_GREATER +global using Lock = object; +#endif + #if NETFRAMEWORK || NETSTANDARD2_0 namespace System { @@ -9,6 +13,32 @@ internal static class HashCode public static int Combine(T1 value1, T2 value2, T3 value3) => (value1, value2, value3).GetHashCode(); } } + +internal static partial class PolyfillExtensions +{ + public static StringBuilder AppendJoin(this StringBuilder builder, char separator, params object?[] values) + { + if (values.Length == 0) + { + return builder; + } + + if (values[0] is {} value) + { + builder.Append(value); + } + + for (var i = 1; i < values.Length; i++) + { + builder.Append(separator); + if (values[i] is {} nextValue) + { + builder.Append(nextValue); + } + } + return builder; + } +} #endif #if NETFRAMEWORK diff --git a/src/Sentry/Internal/ReadOnlyFilesystem.cs b/src/Sentry/Internal/ReadOnlyFilesystem.cs index a5972b58f3..c71792b569 100644 --- a/src/Sentry/Internal/ReadOnlyFilesystem.cs +++ b/src/Sentry/Internal/ReadOnlyFilesystem.cs @@ -17,6 +17,12 @@ public override bool CreateFileForWriting(string path, out Stream fileStream) return false; } + public override bool TryCreateLockFile(string path, out Stream fileStream) + { + fileStream = Stream.Null; + return false; + } + public override bool WriteAllTextToFile(string path, string contents) => false; public override bool MoveFile(string sourceFileName, string destFileName, bool overwrite = false) => false; diff --git a/src/Sentry/Internal/ReadWriteFileSystem.cs b/src/Sentry/Internal/ReadWriteFileSystem.cs index 6a9a08c2a3..ed4b58f496 100644 --- a/src/Sentry/Internal/ReadWriteFileSystem.cs +++ b/src/Sentry/Internal/ReadWriteFileSystem.cs @@ -25,6 +25,26 @@ public override bool CreateFileForWriting(string path, out Stream fileStream) return true; } + /// + /// Tries to create or open a file for exclusive access. + /// + /// + /// This method can throw all of the same exceptions that the constructor can throw. The + /// caller is responsible for handling those exceptions. + /// + public override bool TryCreateLockFile(string path, out Stream fileStream) + { + // Note that FileShare.None is implemented via advisory locks only on macOS/Linux... so it will stop + // other .NET processes from accessing the file but not other non-.NET processes. This should be fine + // in our case - we just want to avoid multiple instances of the SDK concurrently accessing the cache + fileStream = new FileStream( + path, + FileMode.OpenOrCreate, + FileAccess.ReadWrite, + FileShare.None); + return true; + } + public override bool WriteAllTextToFile(string path, string contents) { File.WriteAllText(path, contents); diff --git a/src/Sentry/Sentry.csproj b/src/Sentry/Sentry.csproj index f00de450ff..d753684857 100644 --- a/src/Sentry/Sentry.csproj +++ b/src/Sentry/Sentry.csproj @@ -13,7 +13,7 @@ $(TargetFrameworks);net9.0-ios18.0;net8.0-ios17.0 $(TargetFrameworks);net9.0-maccatalyst18.0;net8.0-maccatalyst17.0 - + @@ -83,7 +83,7 @@ - +