Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] The IAmAnOutbox interface is required by the archiver, but not always registered with IoC #3075

Closed
iancooper opened this issue May 7, 2024 · 12 comments
Labels
3 - Done Bug v9 Required for v9 release v10 Allocal to a v10 release

Comments

@iancooper
Copy link
Member

Describe the bug

When the TimedOutboxArchiver hosted service is initialised, it requests a IAmAnOutbox instance from the IoC container. If a DynamoDb outbox is being used via the UseDynamoDbOutbox extension method, then while IAmAnOutboxSync and IAmAnOutboxAsync both extend the IAmAnOutbox interface, IAmAnOutbox is never registered in the IoC container and so the archiver service fails to initialise, crashing the application

Further technical details

  • Brighter version 9.7.8

Suggested fix

Register the IAmAnOutbox interface in the DynamoDb Outbox registration

V10 differences

V10 has a lot of improvements around Outbox configuration, but we should confirm as part of this bug fix, that the IAmAnOutbox interface is registered in V10 for archiver scenarios

@iancooper iancooper added 0 - Backlog Bug v10 Allocal to a v10 release v9 Required for v9 release labels May 7, 2024
@preardon
Copy link
Member

preardon commented May 7, 2024

I think this is an Odd one, as IAmAnOutbox technically has 0 methods on it, but I agree we should do something

@iancooper
Copy link
Member Author

@preardon The issue comes out of TimedOutboxArchiver taking a dependency on IAmAnOutbox. We then pass that to OutboxArchiver which queries for both interfaces. The alternative would be to have both of the Archiver classes take both IAmAnOutbox* types as dependencies. This was an issue for us as we did not have support for both sync and async for all of our Outboxes at one point. Not sure if that is still true.

@preardon
Copy link
Member

preardon commented May 7, 2024

@iancooper We could, have Sync and Async TimedOutboxArchivers.

One thing (out of scope) that I could use with this as well so some locking (need it for sweeper more than Archiver) but both would be good

@iancooper
Copy link
Member Author

Yeah, came up. For now most folks are using Db locking, but we might want to look at a proper distributed lock

@preardon
Copy link
Member

preardon commented May 7, 2024

I need one at work specifically for the outbox sweeper and archiver let's have a chat about these and the best places to put them on we already have an process lock for sweeping and it might be better to do it at that level instead of the timed service level, but very willing to discuss as I see benefits both Ways

@preardon
Copy link
Member

preardon commented May 7, 2024

Also, I know I didn't say it before but do we want to be registering IAmAnOutbox as we would have to choose which outbox to register against it

@iancooper
Copy link
Member Author

I need one at work specifically for the outbox sweeper and archiver let's have a chat about these and the best places to put them on we already have an process lock for sweeping and it might be better to do it at that level instead of the timed service level, but very willing to discuss as I see benefits both Ways

So, I think that we could look to use this: https://dotnet.github.io/dotNext/features/cluster/index.html and Aspire to help folks deliver easy setup, if we want to support an out-of-process Sweeper/Archiver that does not use a "good enough lock" via a Db

@iancooper
Copy link
Member Author

Also, I know I didn't say it before but do we want to be registering IAmAnOutbox as we would have to choose which outbox to register against it

Easy route there is to do what we do now, query to see if you can cast.

@iancooper
Copy link
Member Author

OK, first stab at this will go with:

  • V9, register the IAmAnOutbox interface. Rely on constructor to query for the IAmAnOutboxSync and IAmAnOutboxAsync interfaces
  • V10 have sync and async sweeper and archiver that take the direct dependency instead.

Let's raise a separate issue for the

@preardon let me know what you think

@preardon
Copy link
Member

@iancooper sounds good

@iancooper
Copy link
Member Author

@preardon

I made the simple change in v9.

Thinking about V10 the difference to the Sweeper is that we don't have the archive methods on the Command Processor. It may be better to move them. The Outbox methods live on the External Service Bus as with other Outbox dependent methods. So by moving the Archiver there, we could probably take a dependency on the External Service Bus in the Archiver instead.

That may trigger a refactoring of ExternalServiceBus, as it has two roles: Producer and Outbox. I don't think we need to split for the consumer, but internally it might shift to two classes.

Let me see what happens when I try this.

preardon added a commit to preardon/Brighter that referenced this issue May 15, 2024
Issue BrighterCommand#3075

This is an example of providers to allow locking across resources,
included is

- In Memory Lock (for when we only want a lock in process)
- Azure Blob Lock
@iancooper
Copy link
Member Author

This has a quick fix in V9 where we register the missing interface. It has a more complete fix in v10 where we move the archiver functionality to bus services; make the archiver a dependency of bus services; and make the timedarchiver take the eventbus services as a dependency. This is more cohesive as the event bus services are the public abstraction over outbox and producer interaction.

See #3090 for V10

preardon added a commit to preardon/Brighter that referenced this issue May 16, 2024
Issue BrighterCommand#3075

This is an example of providers to allow locking across resources,
included is

- In Memory Lock (for when we only want a lock in process)
- Azure Blob Lock
preardon added a commit to preardon/Brighter that referenced this issue May 16, 2024
preardon added a commit to preardon/Brighter that referenced this issue May 16, 2024
Issue BrighterCommand#3075

This is an example of providers to allow locking across resources,
included is

- In Memory Lock (for when we only want a lock in process)
- Azure Blob Lock
preardon added a commit to preardon/Brighter that referenced this issue May 16, 2024
preardon added a commit that referenced this issue May 16, 2024
Issue #3075

This is an example of providers to allow locking across resources,
included is

- In Memory Lock (for when we only want a lock in process)
- Azure Blob Lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Done Bug v9 Required for v9 release v10 Allocal to a v10 release
Projects
None yet
Development

No branches or pull requests

2 participants