From 05f73539251de803f21f9d2dd8fd27019e2c4e73 Mon Sep 17 00:00:00 2001 From: Adam Hathcock Date: Wed, 11 Jun 2025 16:32:06 +0100 Subject: [PATCH] Revert "Merge pull request #335 from specklesystems/adam/cnx-1786-allow-multiple-sends-to-access-sqlite-in-a-non-locking-2" (#339) This reverts commit 59019bf84655579c2bd61969bcbd5fbca959d73f, reversing changes made to 3afaf61a1a95e879fd16521863c7753b90e25128. Co-authored-by: Adam Hathcock --- .../SQLite/SQLiteJsonCacheManager.cs | 8 +- .../V2/MemoryJsonCacheManager.cs | 4 - .../V2/Send/ObjectSaverFactory.cs | 50 ----------- .../Serialisation/V2/Send/SerializeProcess.cs | 1 + .../V2/SerializeProcessFactory.cs | 9 +- src/Speckle.Sdk/ServiceRegistration.cs | 2 - .../DataObjectTests.cs | 2 +- .../DetachedTests.cs | 15 ++-- .../DummyCancellationSqLiteSendManager.cs | 3 - .../Framework/ExceptionSendCacheManager.cs | 1 - .../ObjectSaverFactoryTests.cs | 90 ------------------- .../Framework/DummySqLiteReceiveManager.cs | 3 - .../Framework/DummySqLiteSendManager.cs | 2 - tests/Speckle.Sdk.Testing/MoqTest.cs | 2 +- .../SerializeProcessRecordExceptionTests.cs | 3 + 15 files changed, 21 insertions(+), 174 deletions(-) delete mode 100644 src/Speckle.Sdk/Serialisation/V2/Send/ObjectSaverFactory.cs delete mode 100644 tests/Speckle.Sdk.Serialization.Tests/ObjectSaverFactoryTests.cs diff --git a/src/Speckle.Sdk/SQLite/SQLiteJsonCacheManager.cs b/src/Speckle.Sdk/SQLite/SQLiteJsonCacheManager.cs index b08a83ed..0c3d89f7 100644 --- a/src/Speckle.Sdk/SQLite/SQLiteJsonCacheManager.cs +++ b/src/Speckle.Sdk/SQLite/SQLiteJsonCacheManager.cs @@ -13,18 +13,15 @@ public sealed class SqLiteJsonCacheManager : ISqLiteJsonCacheManager { private readonly CacheDbCommandPool _pool; - public string Path { get; } - public static ISqLiteJsonCacheManager FromMemory(int concurrency) => new SqLiteJsonCacheManager(concurrency); private SqLiteJsonCacheManager(int concurrency) { - Path = ":memory:"; //disable pooling as we pool ourselves var builder = new SqliteConnectionStringBuilder { Pooling = false, - DataSource = Path, + DataSource = ":memory:", Cache = SqliteCacheMode.Shared, Mode = SqliteOpenMode.Memory, }; @@ -37,9 +34,8 @@ public sealed class SqLiteJsonCacheManager : ISqLiteJsonCacheManager private SqLiteJsonCacheManager(string path, int concurrency) { - Path = path; //disable pooling as we pool ourselves - var builder = new SqliteConnectionStringBuilder { Pooling = false, DataSource = Path }; + var builder = new SqliteConnectionStringBuilder { Pooling = false, DataSource = path }; _pool = new CacheDbCommandPool(builder.ToString(), concurrency); Initialize(); } diff --git a/src/Speckle.Sdk/Serialisation/V2/MemoryJsonCacheManager.cs b/src/Speckle.Sdk/Serialisation/V2/MemoryJsonCacheManager.cs index 21395e5f..26e142be 100644 --- a/src/Speckle.Sdk/Serialisation/V2/MemoryJsonCacheManager.cs +++ b/src/Speckle.Sdk/Serialisation/V2/MemoryJsonCacheManager.cs @@ -7,10 +7,6 @@ namespace Speckle.Sdk.Serialisation.V2; public class MemoryJsonCacheManager(ConcurrentDictionary jsonCache) : ISqLiteJsonCacheManager #pragma warning restore CA1063 { -#pragma warning disable CA1065 - public string Path => "MemoryJsonCacheManager"; -#pragma warning restore CA1065 - public IReadOnlyCollection<(string Id, string Json)> GetAllObjects() => jsonCache.Select(x => (x.Key.Value, x.Value.Value)).ToList(); diff --git a/src/Speckle.Sdk/Serialisation/V2/Send/ObjectSaverFactory.cs b/src/Speckle.Sdk/Serialisation/V2/Send/ObjectSaverFactory.cs deleted file mode 100644 index 96ddbcb3..00000000 --- a/src/Speckle.Sdk/Serialisation/V2/Send/ObjectSaverFactory.cs +++ /dev/null @@ -1,50 +0,0 @@ -using System.Collections.Concurrent; -using Microsoft.Extensions.Logging; -using Speckle.InterfaceGenerator; -using Speckle.Sdk.SQLite; -using Speckle.Sdk.Transports; - -namespace Speckle.Sdk.Serialisation.V2.Send; - -public partial interface IObjectSaverFactory : IDisposable; - -[GenerateAutoInterface] -public sealed class ObjectSaverFactory(ILoggerFactory loggerFactory) : IObjectSaverFactory -{ - private readonly ConcurrentDictionary _savers = new(); - - public IObjectSaver Create( - IServerObjectManager serverObjectManager, - ISqLiteJsonCacheManager sqLiteJsonCacheManager, - IProgress? progress, - CancellationToken cancellationToken, - SerializeProcessOptions? options = null - ) - { - if (!_savers.TryGetValue(sqLiteJsonCacheManager.Path, out var saver)) - { - saver = new ObjectSaver( - progress, - sqLiteJsonCacheManager, - serverObjectManager, - loggerFactory.CreateLogger(), - cancellationToken, - options - ); - _savers.TryAdd(sqLiteJsonCacheManager.Path, saver); - } - - return saver; - } - - [AutoInterfaceIgnore] - public void Dispose() - { - foreach (var pool in _savers) - { - pool.Value.Dispose(); - } - - _savers.Clear(); - } -} diff --git a/src/Speckle.Sdk/Serialisation/V2/Send/SerializeProcess.cs b/src/Speckle.Sdk/Serialisation/V2/Send/SerializeProcess.cs index 35cb3731..0254921d 100644 --- a/src/Speckle.Sdk/Serialisation/V2/Send/SerializeProcess.cs +++ b/src/Speckle.Sdk/Serialisation/V2/Send/SerializeProcess.cs @@ -86,6 +86,7 @@ public sealed class SerializeProcess( await WaitForSchedulerCompletion().ConfigureAwait(false); await _highest.DisposeAsync().ConfigureAwait(false); await _belowNormal.DisposeAsync().ConfigureAwait(false); + objectSaver.Dispose(); _processSource.Dispose(); } diff --git a/src/Speckle.Sdk/Serialisation/V2/SerializeProcessFactory.cs b/src/Speckle.Sdk/Serialisation/V2/SerializeProcessFactory.cs index 2bb38bac..c1428bf9 100644 --- a/src/Speckle.Sdk/Serialisation/V2/SerializeProcessFactory.cs +++ b/src/Speckle.Sdk/Serialisation/V2/SerializeProcessFactory.cs @@ -13,7 +13,6 @@ public class SerializeProcessFactory( IObjectSerializerFactory objectSerializerFactory, ISqLiteJsonCacheManagerFactory sqLiteJsonCacheManagerFactory, IServerObjectManagerFactory serverObjectManagerFactory, - IObjectSaverFactory objectSaverFactory, ILoggerFactory loggerFactory ) : ISerializeProcessFactory { @@ -40,7 +39,13 @@ public class SerializeProcessFactory( ) => new SerializeProcess( progress, - objectSaverFactory.Create(serverObjectManager, sqLiteJsonCacheManager, progress, cancellationToken, options), + new ObjectSaver( + progress, + sqLiteJsonCacheManager, + serverObjectManager, + loggerFactory.CreateLogger(), + cancellationToken + ), baseChildFinder, new BaseSerializer(sqLiteJsonCacheManager, objectSerializerFactory), loggerFactory, diff --git a/src/Speckle.Sdk/ServiceRegistration.cs b/src/Speckle.Sdk/ServiceRegistration.cs index 106be8bd..8f8066ae 100644 --- a/src/Speckle.Sdk/ServiceRegistration.cs +++ b/src/Speckle.Sdk/ServiceRegistration.cs @@ -97,8 +97,6 @@ public static class ServiceRegistration typeof(Client) ); serviceCollection.AddMatchingInterfacesAsTransient(typeof(GraphQLRetry).Assembly); - //we want to make object savers be singletons per stream so needs a singleton factory - serviceCollection.AddSingleton(); return serviceCollection; } diff --git a/tests/Speckle.Sdk.Serialization.Tests/DataObjectTests.cs b/tests/Speckle.Sdk.Serialization.Tests/DataObjectTests.cs index 90435f58..da6fdef8 100644 --- a/tests/Speckle.Sdk.Serialization.Tests/DataObjectTests.cs +++ b/tests/Speckle.Sdk.Serialization.Tests/DataObjectTests.cs @@ -41,7 +41,7 @@ public class DataObjectTests new DummyServerObjectManager(), null, default, - new SerializeProcessOptions(false, false, false, true) + new SerializeProcessOptions(true, true, false, true) ); await serializeProcess.Serialize(x); await VerifyJson(json.Single().Value.Value).UseParameters(type); diff --git a/tests/Speckle.Sdk.Serialization.Tests/DetachedTests.cs b/tests/Speckle.Sdk.Serialization.Tests/DetachedTests.cs index 62568110..a8664172 100644 --- a/tests/Speckle.Sdk.Serialization.Tests/DetachedTests.cs +++ b/tests/Speckle.Sdk.Serialization.Tests/DetachedTests.cs @@ -41,7 +41,7 @@ public class DetachedTests objects, null, default, - new SerializeProcessOptions(false, false, false, true) + new SerializeProcessOptions(false, false, true, true) ); await serializeProcess.Serialize(@base); @@ -123,7 +123,7 @@ public class DetachedTests objects, null, default, - new SerializeProcessOptions(false, false, false, true) { MaxParallelism = 1, MaxHttpSendBatchSize = 1 } + new SerializeProcessOptions(false, false, true, true) { MaxParallelism = 1, MaxHttpSendBatchSize = 1 } ); var results = await serializeProcess.Serialize(@base); @@ -150,7 +150,7 @@ public class DetachedTests objects, null, default, - new SerializeProcessOptions(false, false, false, true) { MaxParallelism = 1, MaxHttpSendBatchSize = 1 } + new SerializeProcessOptions(false, false, true, true) { MaxParallelism = 1, MaxHttpSendBatchSize = 1 } ); var results = await serializeProcess.Serialize(@base); @@ -172,7 +172,7 @@ public class DetachedTests objects, null, default, - new SerializeProcessOptions(false, false, false, true) { MaxParallelism = 1, MaxHttpSendBatchSize = 1 } + new SerializeProcessOptions(false, false, true, true) { MaxParallelism = 1, MaxHttpSendBatchSize = 1 } ); var results = await serializeProcess.Serialize(@base); @@ -239,7 +239,7 @@ public class DetachedTests objects, null, default, - new SerializeProcessOptions(false, false, false, true) + new SerializeProcessOptions(false, false, true, true) ); var results = await serializeProcess.Serialize(@base); @@ -272,7 +272,7 @@ public class DetachedTests objects, null, default, - new SerializeProcessOptions(false, false, false, true) + new SerializeProcessOptions(false, false, true, true) ); var results = await serializeProcess.Serialize(@base); await VerifyJsonDictionary(objects); @@ -373,9 +373,6 @@ public class DummyServerObjectManager : IServerObjectManager public class DummySendCacheManager(Dictionary objects) : ISqLiteJsonCacheManager { -#pragma warning disable CA1065 - public string Path => throw new NotImplementedException(); -#pragma warning restore CA1065 public void Dispose() { } public IReadOnlyCollection<(string, string)> GetAllObjects() => throw new NotImplementedException(); diff --git a/tests/Speckle.Sdk.Serialization.Tests/DummyCancellationSqLiteSendManager.cs b/tests/Speckle.Sdk.Serialization.Tests/DummyCancellationSqLiteSendManager.cs index 1244b303..2d6ca588 100644 --- a/tests/Speckle.Sdk.Serialization.Tests/DummyCancellationSqLiteSendManager.cs +++ b/tests/Speckle.Sdk.Serialization.Tests/DummyCancellationSqLiteSendManager.cs @@ -4,9 +4,6 @@ namespace Speckle.Sdk.Serialization.Tests; public class DummyCancellationSqLiteSendManager : ISqLiteJsonCacheManager { -#pragma warning disable CA1065 - public string Path => throw new NotImplementedException(); -#pragma warning restore CA1065 public string? GetObject(string id) => null; public void SaveObject(string id, string json) => throw new NotImplementedException(); diff --git a/tests/Speckle.Sdk.Serialization.Tests/Framework/ExceptionSendCacheManager.cs b/tests/Speckle.Sdk.Serialization.Tests/Framework/ExceptionSendCacheManager.cs index 0f3df346..5572aa02 100644 --- a/tests/Speckle.Sdk.Serialization.Tests/Framework/ExceptionSendCacheManager.cs +++ b/tests/Speckle.Sdk.Serialization.Tests/Framework/ExceptionSendCacheManager.cs @@ -4,7 +4,6 @@ namespace Speckle.Sdk.Serialization.Tests.Framework; public class ExceptionSendCacheManager(bool? hasObject = null, int? exceptionsAfter = null) : ISqLiteJsonCacheManager { - public string Path => "ExceptionSendCacheManager"; private readonly object _lock = new(); private int _count; diff --git a/tests/Speckle.Sdk.Serialization.Tests/ObjectSaverFactoryTests.cs b/tests/Speckle.Sdk.Serialization.Tests/ObjectSaverFactoryTests.cs deleted file mode 100644 index 253c52a2..00000000 --- a/tests/Speckle.Sdk.Serialization.Tests/ObjectSaverFactoryTests.cs +++ /dev/null @@ -1,90 +0,0 @@ -using FluentAssertions; -using Microsoft.Extensions.Logging; -using Moq; -using Speckle.Sdk.SQLite; -using Speckle.Sdk.Testing; - -namespace Speckle.Sdk.Serialisation.V2.Send.Tests; - -public class ObjectSaverFactoryTests : MoqTest -{ - private readonly Mock _loggerFactoryMock; - private readonly Mock> _loggerMock; - private readonly ObjectSaverFactory _factory; - - public ObjectSaverFactoryTests() - { - _loggerFactoryMock = Create(); - _loggerMock = Create>(); - _factory = new ObjectSaverFactory(_loggerFactoryMock.Object); - } - - public override void Dispose() - { - _factory.Dispose(); - base.Dispose(); - } - - [Fact] - public void Create_ShouldReturnObjectSaverInstance() - { - _loggerFactoryMock.Setup(f => f.CreateLogger(typeof(ObjectSaver).FullName)).Returns(_loggerMock.Object); - var cacheManagerMock = Create(); - cacheManagerMock.Setup(x => x.Dispose()); - cacheManagerMock.SetupGet(c => c.Path).Returns("/tmp/test1.db"); - - var saver = _factory.Create( - Create().Object, - cacheManagerMock.Object, - null, - CancellationToken.None - ); - - saver.Should().NotBeNull(); - } - - [Fact] - public void Create_ShouldReturnSameInstanceForSamePath() - { - _loggerFactoryMock.Setup(f => f.CreateLogger(typeof(ObjectSaver).FullName)).Returns(_loggerMock.Object); - var cacheManagerMock = Create(); - cacheManagerMock.Setup(x => x.Dispose()); - cacheManagerMock.SetupGet(c => c.Path).Returns("/tmp/test2.db"); - - var saver1 = _factory.Create( - Create().Object, - cacheManagerMock.Object, - null, - CancellationToken.None - ); - var saver2 = _factory.Create( - Create().Object, - cacheManagerMock.Object, - null, - CancellationToken.None - ); - - saver1.Should().BeSameAs(saver2); - } - - [Fact] - public void Dispose_ShouldDisposeAllSavers() - { - var saverMock1 = Create(); - _factory - .GetType() - .GetField("_savers", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance) - ?.SetValue( - _factory, - new System.Collections.Concurrent.ConcurrentDictionary( - new[] - { - new System.Collections.Generic.KeyValuePair("/tmp/test3.db", saverMock1.Object), - } - ) - ); - saverMock1.Setup(x => x.Dispose()); - - _factory.Dispose(); - } -} diff --git a/tests/Speckle.Sdk.Testing/Framework/DummySqLiteReceiveManager.cs b/tests/Speckle.Sdk.Testing/Framework/DummySqLiteReceiveManager.cs index 01109410..fcfe6832 100644 --- a/tests/Speckle.Sdk.Testing/Framework/DummySqLiteReceiveManager.cs +++ b/tests/Speckle.Sdk.Testing/Framework/DummySqLiteReceiveManager.cs @@ -5,9 +5,6 @@ namespace Speckle.Sdk.Testing.Framework; public sealed class DummySqLiteReceiveManager(IReadOnlyDictionary savedObjects) : ISqLiteJsonCacheManager { -#pragma warning disable CA1065 - public string Path => throw new NotImplementedException(); -#pragma warning restore CA1065 public void Dispose() { } public IReadOnlyCollection<(string, string)> GetAllObjects() => throw new NotImplementedException(); diff --git a/tests/Speckle.Sdk.Testing/Framework/DummySqLiteSendManager.cs b/tests/Speckle.Sdk.Testing/Framework/DummySqLiteSendManager.cs index 97e3f0f5..b1425dd6 100644 --- a/tests/Speckle.Sdk.Testing/Framework/DummySqLiteSendManager.cs +++ b/tests/Speckle.Sdk.Testing/Framework/DummySqLiteSendManager.cs @@ -4,8 +4,6 @@ namespace Speckle.Sdk.Testing.Framework; public class DummySqLiteSendManager : ISqLiteJsonCacheManager { - public string Path => "DummySqLiteSendManager"; - public string? GetObject(string id) => throw new NotImplementedException(); public void SaveObject(string id, string json) => throw new NotImplementedException(); diff --git a/tests/Speckle.Sdk.Testing/MoqTest.cs b/tests/Speckle.Sdk.Testing/MoqTest.cs index bc3ca8fa..34e8391b 100644 --- a/tests/Speckle.Sdk.Testing/MoqTest.cs +++ b/tests/Speckle.Sdk.Testing/MoqTest.cs @@ -8,7 +8,7 @@ public abstract class MoqTest : IDisposable { protected MoqTest() => Repository = new(MockBehavior.Strict); - public virtual void Dispose() => Repository.VerifyAll(); + public void Dispose() => Repository.VerifyAll(); protected MockRepository Repository { get; private set; } = new(MockBehavior.Strict); diff --git a/tests/Speckle.Sdk.Tests.Unit/Serialisation/SerializeProcessRecordExceptionTests.cs b/tests/Speckle.Sdk.Tests.Unit/Serialisation/SerializeProcessRecordExceptionTests.cs index a69a5f76..1e855dd7 100644 --- a/tests/Speckle.Sdk.Tests.Unit/Serialisation/SerializeProcessRecordExceptionTests.cs +++ b/tests/Speckle.Sdk.Tests.Unit/Serialisation/SerializeProcessRecordExceptionTests.cs @@ -21,6 +21,7 @@ public class SerializeProcessRecordExceptionTests : MoqTest .Setup(f => f.CreateLogger("Speckle.Sdk.Serialisation.V2.PriorityScheduler")) .Returns(Create>().Object); var objectSaverMock = Create(); + objectSaverMock.Setup(x => x.Dispose()); var baseChildFinderMock = Create(); var baseSerializerMock = Create(); using var cts = new CancellationTokenSource(); @@ -56,6 +57,7 @@ public class SerializeProcessRecordExceptionTests : MoqTest .Setup(f => f.CreateLogger("Speckle.Sdk.Serialisation.V2.PriorityScheduler")) .Returns(Create>().Object); var objectSaverMock = Create(); + objectSaverMock.Setup(x => x.Dispose()); var baseChildFinderMock = Create(); var baseSerializerMock = Create(); using var cts = new CancellationTokenSource(); @@ -86,6 +88,7 @@ public class SerializeProcessRecordExceptionTests : MoqTest .Setup(f => f.CreateLogger("Speckle.Sdk.Serialisation.V2.PriorityScheduler")) .Returns(Create>().Object); var objectSaverMock = Create(); + objectSaverMock.Setup(x => x.Dispose()); var baseChildFinderMock = Create(); var baseSerializerMock = Create(); using var cts = new CancellationTokenSource();