Reuse ExecutionStrategy instance

[AndriySvyryd]

It can be stored in the DbContext

[martincostello]

I'd be happy to try and pick this up if you could provide a little more detail about the intended design for storing a strategy in the DbContext.

[AndriySvyryd]

Add a field to DbContext
When a new strategy instance is requested and the field is not null then reset its state, return it and set the field to null.
When a strategy instance finishes execution and the field on DbContext is null then store it there.

To encapsulate the field add an interface with a property in .Internal namespace that is implemented explicitly by the DbContext.

[AndriySvyryd]

Alternatively, you can store it in the IExecutionStrategyFactory implementation, add a Return(IExecutionStrategy) to IExecutionStrategyFactory and add IExecutionStrategyFactory to ExecutionStrategyDependencies (setting it in the ExecutionStrategyFactory constructor).

[AndriySvyryd]

The latter approach has better separation of concerns and avoids having to add yet another interface.

[martincostello]

Looking at the code, I don't quite follow the second suggestion. ExecutionStrategyDependencies is used to create an execution strategy, so wouldn't contain an instance of the object itself? Also there appear to be 4 different strategy implementations that would all be updating to be "re-use aware", where as the first option would make re-use an implementation detail in just the one place?

Maybe the new field could be added to DatabaseFacade instead and it cached there?

efcore/src/EFCore/Infrastructure/DatabaseFacade.cs

Lines 266 to 267 in 86c2ad8

	public virtual IExecutionStrategy CreateExecutionStrategy()
	=> Dependencies.ExecutionStrategyFactory.Create();

[martincostello]

Ah, but then Create() is called from other places than DatabaseFacade like StateManager, CosmosClientWrapper etc.

It's not obvious to me what the best place to funnel all the calls through to maximise re-use would be.

[AndriySvyryd]

ExecutionStrategyDependencies is used to create an execution strategy, so wouldn't contain an instance of the object itself?

Yes, that's why it needs to be set from the IExecutionStrategyFactory implementation constructor.

Also there appear to be 4 different strategy implementations that would all be updating to be "re-use aware", where as the first option would make re-use an implementation detail in just the one place?

Separate implementations is not a downside. For example, NonRetryingExecutionStrategy doesn't contain any state, so ExecutionStrategyFactory could just always return a singleton instance.