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

Initial Play with Scoped Handlers in a Singleton Command Processor #1622

Conversation

preardon
Copy link
Member

@preardon preardon commented Jul 5, 2021

@iancooper @holytshirt

Hey Guys, I had a think over the weekend and came up with this, I know that its not an elegant solution however it works and is a talking point and does allow us to register the Command Processor as a Singleton, essentially I have wrapped the Singleton Command Processor with a Scoped "Scoped Command Processor" (Working Title) which has the Singleton as a Dependency and injects the HandlerFactory for any Scoped Handler Factories you may need, I've updated my Example and it works, I have not done anything like this for the outbox as yet.

Also added an extension (.UseScopedCommandProcessor) so it is not added if not needed, and should be fairly easy to Implement for other DI providers

@iancooper
Copy link
Member

iancooper commented Jul 5, 2021

It is interesting. I mean, it does suggest that an alternative is to break up CommandProcessor into two parts. The parts that whose lifetime can be transient or scoped and the parts whose lifetime must be a singleton, and inject every scoped or transient with the singleton of the long-running state. Mostly that is memento's for pipelines, background processing etc.

So perhaps:

  • CommandProcessor
  • CommandProcessorService

Where Service is the long running elements that need to be injected to the ephemeral instances? I think I slightly prefer that way over the scoped command processor because it works across a range of scenarios (including making both a singleton if you want). It would be fairly easy to document that the CommandProcessorService needed to be a singleton.

@holytshirt ?

@preardon
Copy link
Member Author

preardon commented Jul 5, 2021

@iancooper I was completely thinking that the wrapper would be Optional, but I have no objections to it being split into two parts like this, and thinking about this, it may be a little cleaner

@iancooper
Copy link
Member

@iancooper I was completely thinking that the wrapper would be Optional, but I have no objections to it being split into two parts like this, and thinking about this, it may be a little cleaner

Do you want to have a stab at that? Or clearer if I do it? I am good either way.

@preardon
Copy link
Member Author

preardon commented Jul 5, 2021

I'm happy to have a go at it, I'll Update this PR with it,

Just to be clear the Current CommandProcessor will become the "CommandProcessorService" and my ScopedCommandProcessor will become the "CommandProcessor" and can be registered either as a Singleton or Scoped (I suppose Transient could be fine as well), I suppose that this would mean that both of them would live in Core

@iancooper
Copy link
Member

I'm happy to have a go at it, I'll Update this PR with it,

Just to be clear the Current CommandProcessor will become the "CommandProcessorService" and my ScopedCommandProcessor will become the "CommandProcessor" and can be registered either as a Singleton or Scoped (I suppose Transient could be fine as well), I suppose that this would mean that both of them would live in Core

Possibly.

I would be looking at - what fields/events/threads need a lifetime longer than transient or request response. Move all that to CommandProcessorService with an ICommandProcessorService interface. Inject that into ICommandProcessor, which should probably have only state that is dependent on a scope, or explicitly transient. If something could be either, I might push it to the Service for now, so only state in the CommandProcessor that we want to make it possible to flush with every request-response etc.

That might be the same destination, just another route. Give it a stab and I'll give you feedback.

@preardon
Copy link
Member Author

preardon commented Jul 5, 2021

@iancooper just pushed a second commit, still early days however this only registers the Command Process Service as a singleton and uses the old flag to register the new Command Processor as Scoped / Singleton, I've done some light testing and it works, One downside that I have found is that for Request Reply queues you do need a Non Scoped Handler Factory, not sure if this is going to be a big issue (I Suppose it all depends on what you want to do with the call back).

If you are Happy with this approach in principal I will look into what I can do to allow the Outbox to Operate in this way (when required)

@iancooper
Copy link
Member

iancooper commented Jul 5, 2021

If you are Happy with this approach in principal I will look into what I can do to allow the Outbox to Operate in this way (when required)

This would work, but I was wondering about something slightly more radical. Under this edit, Send is implemented in CommandProcessorService, and we pass through the HandlerFactory i.e. that which varies with Scope.

I was thinking more that Send would still be on the CommandProcessor but that singleton based state, such as the memento etc would live in the CommandProcessorService.

The CommandProcessorService would internal, as its a detail of CommandProcessor at the point. Here is what I would do:

  • Move all the state into CommandProcessorService as public properties (it's an internal class don't forget).
  • Move all the private methods to CommandProcessorService as public methods
  • Figure out if there is state that the CommandProcessor needs in the Send methods outside the calls to CommandProcessorService, try to move that into CommandProcessorService as a method call (hard bit).
  • Make this compile, get the split working with all the tests passing with the impl details moved into this component
  • Now give CommandProcessor fields for anything that needs to be scoped Session or Transient
  • Provide an Init() method for CommandProcessor that returns a CallContext that implements IDisposable (make it a private class to CommandProcessorService - we want it to access parent fields etc but caller just needs semantics of IDispose)
  • Pass the scope dependent variables to Init, and initialize the fields on CommandProcessorService (you can make these private now - they should only be set this way)
  • On Dispose of the CallContext, have it set those fields to null i.e. we don't keep between invocations
  • Wrap details in the calls in CommandProcessor (Send, Publish) within that context i.e. a using statement
  • Check all the tests pass

That means the Service now truly provides 'services' to the CommandProcessor and is passed in the state that varies with each call.

I am happy to create a separate PR that shows this, if you would find that clearer than this list. Or hop on a call and pair if we can find some time.

@iancooper
Copy link
Member

iancooper commented Jul 6, 2021

@preardon ^^^^ (forgot to tag you)

I'll see if I can get a prototype branch at lunchtime today, so you can see what I mean.

@preardon
Copy link
Member Author

preardon commented Jul 6, 2021

@iancooper Not a problem mate, I think I have a halfway decent idea now (wasn't initially sure how Ballsy to go) but always happy to have an example :) I'll should be able to have a good whack at this in the next few days

@iancooper
Copy link
Member

Going to close this PR, I think we can focus on the EventServicesBus route. But this really helped spark that direction, so thanks.

@iancooper iancooper closed this Jul 28, 2021
@preardon preardon deleted the issue/SingletonCommandProcessor branch May 1, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants