EventStore repository to allow connection factory to be passed

[ninjaboy]

This change introduces connection factory method as a dependency to the EventStore repository instead of connection object to allow more flexibility with regards to how connection object is resolved. (e.g. to allow connection polling, re-connection and monitoring)

Work done

1. Major changes

Changed EventStoreRepository constructor from
public EventStoreRepository(IHoldAllConfiguration configs, IEventStoreConnection connection)
to
public EventStoreRepository(IHoldAllConfiguration configs, Func<IEventStoreConnection> connectionFactory)

2. Important things to review

EventStoreRepository as the method to create new session was changed

Integrity

• I have had a look on the PR preview for obvious whitespace\formatting issues before actually openned this PR
• I have run unit tests
• I have run EventStore integration tests

[iblazhko]

ArgumentNullException is not appropriate here - connection is a result, not an argument.
InvalidOperationException maybe, or do not passing anything as InnerException if there is no exception

[ninjaboy]

Valid point

[ninjaboy]

Removed InnerException at all

[iblazhko]

I think there is a problem with connection lifecycle.
Connection is created by calling the factory in the EventstoreRepository, but the connection is never disposed of explicitly.

I propose to pass factory into EventStoreSession, create new instance of the connection in the EventStoreSession constructor, and dispose the connection in the session's Dispose:

public class EventStoreSession<TState> : BaseEventSourcedSession<TState, int>
{
...
        public EventStoreSession(IHoldAllConfiguration configuration,
                                 Func<IEventStoreConnection> eventStoreConnectionFactory,
                                 string streamName)
            : base(configuration)
        {
            this.eventStoreConnection = eventStoreConnectionFactory(); // check for null factory before that, move error handling from repository here
            this.streamName = streamName ?? throw new ArgumentNullException(nameof(streamName));
        }

        protected override void Dispose(bool disposing)
        {
            if (disposing)
            {
                ConsiderSessionDisposed();
                eventStoreConnection.Dispose();
            }
            base.Dispose(disposing);
        }
}

[ninjaboy]

@iblazhko , thanks for the spot Ivan, I have addressed your comment

[iblazhko]

throw at L40 will leave the connection open.

[iblazhko]

This cleanup function could potentially be shared between EventStoreRepository and EventStoreSession

[ninjaboy]

Done

[skleanthous]

@iblazhko & @ninjaboy I am a bit worried about this API as it is now. The reason is that DI frameworks like Autofac will emit a Func for you even if you register a connection as single instance (see https://autofaccn.readthedocs.io/en/latest/advanced/delegate-factories.html). In this case if an instance is disposed, then any subsequent calls to the factory method will return a disposed version of the connection.

What I think would be best is to implement a factory of our own that explicitly manages instantiation of the connection and its lifetime.

[ninjaboy]

@skleanthous , Shall explore the following approach then:

EventStoreRepository repository; //injected via ctor
void Operation(string id){
   var connection = GetConnection(); //client decides whether it's from pool or a new one or whatever
   using(repository.BeginSessionForId(connection, id)){
       //do stuff...
   }
   //now I decide what to do with the connection
   connection.Close();
}

[skleanthous]

@ninjaboy Something like connectionFactory.GetSingleInstance() vs connectionFactory.GetTransient()? If so then yeah, that's perfect.

[ninjaboy]

@skleanthous , GetConnection() -> This is up for a client to decide. They may choose whatever strategy to manage the connection (e.g. reuse between sessions, or open close or whatever, the session only requires the connection to be open.

However, looking at the interface that repository currently implements I won't be able to pass arbitrary parameters to the session. Because I have to stick to:

    public interface IStartSessions<in TEntitySelector, TState>
    {
        Task<IManageSessionOf<TState>> BeginSessionFor(TEntitySelector selector, bool throwIfNotExists);
        Task Delete(TEntitySelector selector);
        Task<bool> Contains(TEntitySelector selector);
    }

[ninjaboy]

I must say that I am really confused so far with how the issue needs to be resolved.
Here are some links that I am trying to follow to figure out the possible solution:
EventStore/EventStore#1584
EventStore/EventStore#1732
https://groups.google.com/forum/#!topic/event-store/nkgzXpy56NQ
https://groups.google.com/forum/#!topic/event-store/xu1GQzZtCiM
https://groups.google.com/forum/#!topic/event-store/z0RWBnBesoU

Any suggestion on how we shall handle the problem with connection object going into the Disposed state are welcome.

[ninjaboy]

https://groups.google.com/forum/#!topic/event-store/nkgzXpy56NQ
This post is particularly helpful and in the end it mentions the change introduced in v4.1.1 that introduces a function SetMaxQueueTimeout that looks ismilar to what we need. However .net core version of event store client exists only for version 4.1.0.23 and the new call is introduced in v 4.1.1.

[ninjaboy]

BTW, I tried EventStore.Client.NetCore.4.1.1-rc and SetMaxQueueTimeout but it still doesn't work as expected with KeepReconnecting

[skleanthous]

Will be done in a future release