diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e7ef5b50d..f30103801a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ ### Features +- Implemented 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)) - The SDK now makes use of the new SessionEndStatus `Unhandled` when capturing an unhandled but non-terminal exception, i.e. through the UnobservedTaskExceptionIntegration ([#4633](https://github.com/getsentry/sentry-dotnet/pull/4633), [#4653](https://github.com/getsentry/sentry-dotnet/pull/4653)) - The SDK now provides a `IsSessionActive` to allow checking the session state ([#4662](https://github.com/getsentry/sentry-dotnet/pull/4662)) - The SDK now makes use of the new SessionEndStatus `Unhandled` when capturing an unhandled but non-terminal exception, i.e. through the UnobservedTaskExceptionIntegration ([#4633](https://github.com/getsentry/sentry-dotnet/pull/4633)) diff --git a/src/Sentry/GlobalSessionManager.cs b/src/Sentry/GlobalSessionManager.cs index 8fb6cb8413..cfa27fe89b 100644 --- a/src/Sentry/GlobalSessionManager.cs +++ b/src/Sentry/GlobalSessionManager.cs @@ -33,10 +33,7 @@ public GlobalSessionManager( _clock = clock ?? SystemClock.Clock; _persistedSessionProvider = persistedSessionProvider ?? (filePath => Json.Load(_options.FileSystem, filePath, PersistedSessionUpdate.FromJson)); - - // 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.GetIsolatedCacheDirectoryPath(); } // 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..3f8cb09cfd --- /dev/null +++ b/src/Sentry/Internal/CacheDirectoryCoordinator.cs @@ -0,0 +1,146 @@ +using Sentry.Extensibility; +using Sentry.Internal.Extensions; + +namespace Sentry.Internal; + +internal class CacheDirectoryCoordinator : IDisposable +{ + private readonly IDiagnosticLogger? _logger; + private readonly IFileSystem _fileSystem; + private readonly Lock _lock = new(); + + private Stream? _lockStream; + private readonly string _lockFilePath; + + private volatile bool _acquired; + private volatile bool _disposed; + + public CacheDirectoryCoordinator(string cacheDir, IDiagnosticLogger? logger, IFileSystem fileSystem) + { + _logger = logger; + _fileSystem = fileSystem; + // Note this creates a lock file in the cache directory's parent directory... not in the cache directory itself + _lockFilePath = $"{cacheDir}.lock"; + } + + public bool TryAcquire() + { + if (_acquired) + { + return true; + } + + lock (_lock) + { + if (_acquired) + { + return true; + } + + if (_disposed) + { + return false; + } + + try + { + var baseDir = Path.GetDirectoryName(_lockFilePath); + if (!string.IsNullOrWhiteSpace(baseDir)) + { + _fileSystem.CreateDirectory(baseDir); + } + _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() + { + if (_disposed) + { + return; + } + + lock (_lock) + { + _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.ToString(), + options.InitCounter.Count.ToString()); +#endif + return stringBuilder.ToString(); + } + + internal static string? GetIsolatedCacheDirectoryPath(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 f2117f60b2..ebb4c9dab4 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/src/Sentry/Internal/Http/CachingTransport.cs @@ -5,26 +5,35 @@ 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 { + internal const int ResultMigrationOk = 0; + internal const int ResultMigrationNoBaseCacheDir = 1; + internal const int ResultMigrationAlreadyMigrated = 2; + internal const int ResultMigrationLockNotAcquired = 3; + internal const int ResultMigrationCancelled = 4; + private const string EnvelopeFileExt = "envelope"; private const string ProcessingFolder = "__processing"; 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 +54,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 +85,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.GetIsolatedCacheDirectoryPath() ?? 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 +105,19 @@ 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); + MigrateVersion5Cache(_workerCts.Token); + }, _workerCts.Token); } else { @@ -178,6 +197,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 +255,106 @@ 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"); + } + } + + /// + /// When migrating from version 5 to version 6, the cache directory structure changed. + /// In version 5, there were no isolated subdirectories. + /// + /// + /// An integer return code to facilitate testing. + internal int MigrateVersion5Cache(CancellationToken cancellationToken) + { + if (_options.GetBaseCacheDirectoryPath() is not { } baseCacheDir) + { + return ResultMigrationNoBaseCacheDir; + } - const int maxAttempts = 3; - for (var attempt = 1; attempt <= maxAttempts; attempt++) + // This code needs to execute only once during the first initialization of version 6. + var markerFile = Path.Combine(baseCacheDir, ".migrated"); + if (_fileSystem.FileExists(markerFile)) + { + return ResultMigrationAlreadyMigrated; + } + + var migrationLock = Path.Combine(baseCacheDir, "migration"); + using var coordinator = new CacheDirectoryCoordinator(migrationLock, _options.DiagnosticLogger, _options.FileSystem); + if (!coordinator.TryAcquire()) + { + return ResultMigrationLockNotAcquired; + } + + const string searchPattern = $"*.{EnvelopeFileExt}"; + var processingDirectoryPath = Path.Combine(baseCacheDir, ProcessingFolder); + foreach (var cacheDir in new[] { baseCacheDir, processingDirectoryPath }) + { + if (cancellationToken.IsCancellationRequested) { - try + return ResultMigrationCancelled; // graceful exit on cancellation + } + + if (!_fileSystem.DirectoryExists(cacheDir)) + { + continue; + } + + foreach (var filePath in _fileSystem.EnumerateFiles(cacheDir, searchPattern, SearchOption.TopDirectoryOnly)) + { + if (cancellationToken.IsCancellationRequested) { - _fileSystem.MoveFile(filePath, destinationPath); - break; + return ResultMigrationCancelled; // graceful exit on cancellation } - 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); + var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath)); + _options.LogDebug("Migrating version 5 cache file: {0} to {1}.", filePath, destinationPath); + MoveFileWithRetries(filePath, destinationPath, "version 5"); + } + } - 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); - } + // Leave a marker file so we don't try to migrate again + _fileSystem.WriteAllTextToFile(markerFile, "6.0.0"); + return ResultMigrationOk; + } + + private void MoveFileWithRetries(string filePath, string destinationPath, string moveType) + { + const int maxAttempts = 3; + for (var attempt = 1; attempt <= maxAttempts; attempt++) + { + try + { + _fileSystem.MoveFile(filePath, destinationPath); + + break; + } + catch (Exception ex) + { + if (!_fileSystem.FileExists(filePath)) + { + _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; + } + + 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 +403,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 +664,19 @@ public Task StopWorkerAsync() return _worker; } + private Task StopRecyclerAsync() + { + if (_recycler?.IsCompleted != false) + { + // already stopped + return Task.CompletedTask; + } + + _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 +695,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 ed8451dbfa..dfcc6310f5 100644 --- a/src/Sentry/Sentry.csproj +++ b/src/Sentry/Sentry.csproj @@ -83,7 +83,7 @@ - +