Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ public abstract class BasePersistence
protected static void RegisterDataStores(IServiceCollection services)
{
services.AddSingleton(TimeProvider.System);
services.AddSingleton<MinimumRequiredStorageState>();

services.AddSingleton<IServiceControlSubscriptionStorage, SubscriptionStorage>();
services.AddSingleton<ISubscriptionStorage>(p => p.GetRequiredService<IServiceControlSubscriptionStorage>());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,35 @@
namespace ServiceControl.Persistence.EFCore.Implementation.UnitOfWork;

using Microsoft.Extensions.DependencyInjection;
using ServiceControl.Persistence.EFCore.Abstractions;
using ServiceControl.Persistence.EFCore.DbContexts;
using ServiceControl.Persistence.EFCore.Infrastructure;
using ServiceControl.Persistence.UnitOfWork;

public class EFIngestionUnitOfWork : IIngestionUnitOfWork
{
readonly ServiceControlDbContext dbContext;
readonly AsyncServiceScope scope;

public EFIngestionUnitOfWork(AsyncServiceScope scope, ServiceControlDbContext dbContext, IBodyStoragePersistence storagePersistence, EFPersisterSettings settings)
{
this.scope = scope;
this.dbContext = dbContext;
}

public IMonitoringIngestionUnitOfWork Monitoring =>
throw new NotImplementedException();

public IRecoverabilityIngestionUnitOfWork Recoverability =>
throw new NotImplementedException();

public Task Complete(CancellationToken cancellationToken) =>
throw new NotImplementedException();
public Task Complete(CancellationToken cancellationToken) => dbContext.SaveChangesAsync(cancellationToken);

public void Dispose()
public async ValueTask DisposeAsync()
{
// Nothing to dispose yet
await dbContext.DisposeAsync();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'scope.dispose' on the next line should already take care of dbcontext

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this comment is implemented then you don't actually know that anymore

await scope.DisposeAsync();

GC.SuppressFinalize(this);
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
namespace ServiceControl.Persistence.EFCore.Implementation.UnitOfWork;

using Microsoft.Extensions.DependencyInjection;
using ServiceControl.Persistence.EFCore.Abstractions;
using ServiceControl.Persistence.EFCore.DbContexts;
using ServiceControl.Persistence.EFCore.Infrastructure;
using ServiceControl.Persistence.UnitOfWork;

public class EFIngestionUnitOfWorkFactory : IIngestionUnitOfWorkFactory
public class EFIngestionUnitOfWorkFactory(
IServiceProvider serviceProvider,
MinimumRequiredStorageState storageState,
IBodyStoragePersistence storagePersistence) : IIngestionUnitOfWorkFactory
{
public ValueTask<IIngestionUnitOfWork> StartNew() =>
throw new NotImplementedException();
public ValueTask<IIngestionUnitOfWork> StartNew()
{
var scope = serviceProvider.CreateAsyncScope();
var dbContext = scope.ServiceProvider.GetRequiredService<ServiceControlDbContext>();
var settings = scope.ServiceProvider.GetRequiredService<EFPersisterSettings>();
var unitOfWork = new EFIngestionUnitOfWork(scope, dbContext, storagePersistence, settings);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are passing the container reference into the unit of work it seems like you could just resolve everything inside it's own constructor

However I'd recommend instead passing it in as an (optional?) IDisposable since that's all it cares about. An added bonus here is that it also removes the depend on Microsoft.Extensions.DI and makes testing it not require a container.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll do this in the next PR

return ValueTask.FromResult<IIngestionUnitOfWork>(unitOfWork);
}

public bool CanIngestMore() =>
throw new NotImplementedException();
public bool CanIngestMore() => storageState.CanIngestMore;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
namespace ServiceControl.Persistence.EFCore.Infrastructure;

using System;
using System.Threading;
using System.Threading.Tasks;

public interface IBodyStoragePersistence
{
Task WriteBody(string bodyId, DateTime createdOn, ReadOnlyMemory<byte> body, string contentType, CancellationToken cancellationToken = default);
Task<MessageBodyFileResult?> ReadBody(string bodyId, DateTime createdOn, CancellationToken cancellationToken = default);
Task DeleteBody(string bodyId, CancellationToken cancellationToken = default);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
namespace ServiceControl.Persistence.EFCore.Infrastructure;

using System.IO;

public class MessageBodyFileResult

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the 'required' modifier for the props?

{
public Stream Stream { get; set; } = null!;
public string ContentType { get; set; } = null!;
public int BodySize { get; set; }
public string Etag { get; set; } = null!;
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public async Task SingleMessageMarkedAsArchiveShouldExpire()
{
var (context, attempt) = CreateMessageContext();

using (var uow = await IngestionUnitOfWorkFactory.StartNew())
await using (var uow = await IngestionUnitOfWorkFactory.StartNew())
{
await uow.Recoverability.RecordFailedProcessingAttempt(context, attempt, []);

Expand Down Expand Up @@ -63,7 +63,7 @@ public async Task AllMessagesInUnArchivedGroupShouldNotExpire()
var (contextA, attemptA) = CreateMessageContext();
var (contextB, attemptB) = CreateMessageContext();

using (var uow = await IngestionUnitOfWorkFactory.StartNew())
await using (var uow = await IngestionUnitOfWorkFactory.StartNew())
{
await uow.Recoverability.RecordFailedProcessingAttempt(contextA, attemptA, [new FailedMessage.FailureGroup { Id = groupIdA }]);
await uow.Recoverability.RecordFailedProcessingAttempt(contextB, attemptB, [new FailedMessage.FailureGroup { Id = groupIdB }]);
Expand Down Expand Up @@ -100,7 +100,7 @@ public async Task AllMessagesInArchivedGroupShouldExpire()
var groupId = Guid.NewGuid().ToString();
var (context, attempt) = CreateMessageContext();

using (var uow = await IngestionUnitOfWorkFactory.StartNew())
await using (var uow = await IngestionUnitOfWorkFactory.StartNew())
{
await uow.Recoverability.RecordFailedProcessingAttempt(context, attempt, [new FailedMessage.FailureGroup { Id = groupId }]);

Expand All @@ -123,7 +123,7 @@ public async Task SingleMessageMarkedAsResolvedShouldExpire()
{
var (context, attempt) = CreateMessageContext();

using (var uow = await IngestionUnitOfWorkFactory.StartNew())
await using (var uow = await IngestionUnitOfWorkFactory.StartNew())
{
await uow.Recoverability.RecordFailedProcessingAttempt(context, attempt, []);

Expand All @@ -146,7 +146,7 @@ public async Task RetryConfirmationProcessingShouldTriggerExpiration()
{
var (context, attempt) = CreateMessageContext();

using (var uow = await IngestionUnitOfWorkFactory.StartNew())
await using (var uow = await IngestionUnitOfWorkFactory.StartNew())
{
await uow.Recoverability.RecordFailedProcessingAttempt(context, attempt, []);

Expand All @@ -159,7 +159,7 @@ public async Task RetryConfirmationProcessingShouldTriggerExpiration()

Assert.That(errors.Results, Has.Count.EqualTo(1), "Failed message should be available to query after ingestion");

using (var uow = await IngestionUnitOfWorkFactory.StartNew())
await using (var uow = await IngestionUnitOfWorkFactory.StartNew())
{
await uow.Recoverability.RecordSuccessfulRetry(errors.Results.First().Id);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ async Task RunTest(Func<Dictionary<string, string>, string> getIdToQuery)
};

using (var cancellationSource = new CancellationTokenSource())
using (var uow = await ingestionFactory.StartNew())
await using (var uow = await ingestionFactory.StartNew())
{
var context = new MessageContext(messageId, headers, body, new TransportTransaction(), "receiveAddress", new NServiceBus.Extensibility.ContextBag());
var processingAttempt = new FailedMessage.ProcessingAttempt
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public async Task Unit_of_work_detects_endpoint()
EndpointDetails = new EndpointDetails { Host = "Host1", HostId = Guid.NewGuid(), Name = "Endpoint" }
};

using (var unitOfWork = await UnitOfWorkFactory.StartNew())
await using (var unitOfWork = await UnitOfWorkFactory.StartNew())
{
await unitOfWork.Monitoring.RecordKnownEndpoint(knownEndpoint);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,16 @@ public override Task Complete(CancellationToken cancellationToken)
fallback.Complete(cancellationToken)
);

protected override void Dispose(bool disposing)
protected override async ValueTask DisposeAsyncCore()
{
if (disposing)
if (primary != null)
{
primary?.Dispose();
fallback?.Dispose();
await primary.DisposeAsync();
}

if (fallback != null)
{
await fallback.DisposeAsync();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
using System.Threading;
using System.Threading.Tasks;

public interface IIngestionUnitOfWork : IDisposable
public interface IIngestionUnitOfWork : IAsyncDisposable
{
IMonitoringIngestionUnitOfWork Monitoring { get; }
IRecoverabilityIngestionUnitOfWork Recoverability { get; }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,25 +1,21 @@
namespace ServiceControl.Persistence.UnitOfWork
namespace ServiceControl.Persistence.UnitOfWork
{
using System;
using System.Threading;
using System.Threading.Tasks;

public abstract class IngestionUnitOfWorkBase : IIngestionUnitOfWork
{
protected virtual void Dispose(bool disposing)
{
}
protected virtual ValueTask DisposeAsyncCore() => ValueTask.CompletedTask;

public void Dispose()
public async ValueTask DisposeAsync()
{
Dispose(true);
await DisposeAsyncCore();
GC.SuppressFinalize(this);
}

~IngestionUnitOfWorkBase() => Dispose(false);

public IMonitoringIngestionUnitOfWork Monitoring { get; protected set; }
public IRecoverabilityIngestionUnitOfWork Recoverability { get; protected set; }
public virtual Task Complete(CancellationToken cancellationToken) => Task.CompletedTask;
}
}
}
2 changes: 1 addition & 1 deletion src/ServiceControl/Operations/ErrorIngestor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ async Task<IReadOnlyList<MessageContext>> PersistFailedMessages(List<MessageCont

try
{
using var unitOfWork = await unitOfWorkFactory.StartNew();
await using var unitOfWork = await unitOfWorkFactory.StartNew();
var storedFailedMessageContexts = await errorProcessor.Process(failedMessageContexts, unitOfWork);
await retryConfirmationProcessor.Process(retriedMessageContexts, unitOfWork);

Expand Down