BrighterCommand / Brighter

A framework for building messaging apps with .NET and C#.
https://www.goparamore.io/
MIT License
2.03k stars 257 forks source link

Bug: DynamoDbOutbox Archiver Missing Topics #3351

Open jtsalva opened 1 month ago

jtsalva commented 1 month ago

Describe the bug

Current implementation depends on DynamoDbOutbox._topicName being populated as messages are added to the outbox. The archiver is trying to find messages through the async outbox but the sync version is used when adding to the outbox.

services
    .AddBrighter()
    .UseExternalBus(...)
    .AutoFromAssemblies()
    .UseDynamoDbOutbox() <---- registers 3 instances of the outbox, IAmAnOutbox, IAmAnOutboxSync, IAmAnOutboxAsync
    .UseOutboxSweeper() <---- uses non-async outbox
    .UseOutboxArchiver(new NullOutboxArchiveProvider()); <---- uses async outbox

Proposed fix

Short term Make DynamoDbOutbox._topicNames static

Long term Maybe it should be possible to derive topic names from publications so that the archival process can run without needing any messages added first

Further technical details

dhickie commented 1 month ago

I can't see a good reason for having three instances of the outbox registered, so to me the most sensible option would be to ensure only a single instance of the dynamo outbox is registered for all three interfaces.

One possible issue there is that if you create a DynamoDbOutbox instance inside UseDynamoDbOutbox and add that instance directly, then the method can't be called until IAmazonDynamoDB has been registered, instead of just relying that it's going to be registered at some point. So perhaps, then, BuildDynamoDbOutbox should, instead of automatically creating a new outbox instance, try to retrieve the implementation of IAmAnOutbox and then return that instance if it exists. Otherwise return a new instance.

iancooper commented 1 month ago

Maybe it should be possible to derive topic names from publications so that the archival process can run without needing any messages added first

In V10 we have the Topic on the Publication, we could backport this to V9, as it is additive and not a breaking change.

iancooper commented 1 month ago

I can't see a good reason for having three instances of the outbox registered, so to me the most sensible option would be to ensure only a single instance of the dynamo outbox is registered for all three interfaces. Agreed, that is an error. It implements all three for us.

iancooper commented 1 month ago

BuildDynamoDbOutbox should, instead of automatically creating a new outbox instance, try to retrieve the implementation of IAmAnOutbox and then return that instance if it exists. Otherwise return a new instance. Yeah, it sounds reasonable that we don't rebuild if we have built it once already. Effectively it is a singleton. In fact, ExternalBusService pretty much enforces that.

jtsalva commented 1 month ago

@dhickie @iancooper I've raised #3358 as suggested

iancooper commented 1 month ago

@jtsalva Approved and merged. Do we need to fix this in V9 as well?

jtsalva commented 3 weeks ago

@iancooper the merged PR was for v9. I had a look at the v10 master branch and I can't find UseDynamoDbOutbox there