Optimistic concurrency control by NES and NeventStore+SQL

[lees-oz]

Hi there,
I've noticed that when the stream is commited, NES performs a read first to detect concurrency:

using (IEventStream eventStream = this._eventStore.OpenStream(bucketId, id, version, int.MaxValue))
    {
        EventStoreAdapter.Logger.Debug("Opened stream has StreamRevision {0}", (object) eventStream.StreamRevision);
        if (version != eventStream.StreamRevision && Transaction.Current != (Transaction) null)
        { 
            throw new ConflictingCommandException

Still there's a possibility for concurrency issue this way (result of OpenStream becomes stale immediately). I am using NEventStore with SQL persistence and the consistency of event stream is guaranteed by the database (PK on bucket, streamId and version). So the OpenStream looks like a redundant action in that case for me.
Please comment on that, thank you!

[marinkobabic]

@lees-oz
Before we can do a commit, we will open then stream

using (var stream = _eventStore.OpenStream(bucketId, id, version, int.MaxValue))

The version here says, which the minimum version of the stream should be. So when you attend to open version 2, maybe the opened stream version will be 3, but you expected to be 2, because you made your business decisions based on the state of the version 2. Therefore is the check there. Do you agree?

[lees-oz]

Not completely, because between the moment you do the check and the moment you persist stream there's a possibility that version 3 will be added by concurrent thread. So that check cannot guarantee the event stream consistency in the storage. In case of SQL storage, SQL only can guarantee that (with PK mechanism). Then I get that exception:

NES.ConflictingCommandException: Cannot insert duplicate key row in object 'dbo.Commits' with unique index 'IX_Commits_CommitSequence'. The duplicate key value is (default, EE855B7AC9741EB9306D59FC5102297DEB0EC678, 3). ---> NEventStore.ConcurrencyException: Cannot insert duplicate key row in object 'dbo.Commits' with unique index 'IX_Commits_CommitSequence'. The duplicate key value is (default, EE855B7AC9741EB9306D59FC5102297DEB0EC678, 3). ---> NEventStore.Persistence.Sql.UniqueKeyViolationException: Cannot insert duplicate key row in object 'dbo.Commits' with unique index 'IX_Commits_CommitSequence'. The duplicate key value is (default, EE855B7AC9741EB9306D59FC5102297DEB0EC678, 3). ---> System.Data.SqlClient.SqlException: Cannot insert duplicate key row in object 'dbo.Commits' with unique index 'IX_Commits_CommitSequence'. The duplicate key value is (default, EE855B7AC9741EB9306D59FC5102297DEB0EC678, 3)

If this check in NES detects the concurrency conflict, I get same exception type, with a different message and stacktrace though:

NES.ConflictingCommandException: EventSource a1d896f6-11ff-470e-a815-a65500cf078a has the version 17 and the stream has version 19 

Those 2 exceptions mean same and it's not clear for me why distinguish them, especially if that costs another OpenStream operation.

[marinkobabic]

You are right. Let's check some examples:

Example 1:
we open stream 2
we have our aggregate object populated
we close stream 2
job is done, so lets write
we open stream 2
another thread writes stream 3
we write stream 3
exception by database because of duplicate key

This is totally fine, our additional check not needed.

Example 2:
we open stream 2
we have our aggregate object populated
we close stream 2
job is done, so lets write
another thread writes stream 3
we open stream 3
we write stream 4

Here we have no database exception, because we have no conflict.

In second example we made our business decision based on stream 2 and not stream 3, therefore the check. Do you agree now?

[lees-oz]

Probably I know where the confusion comes from. You have to open the stream(s) second time, when you Commit unit of work. Because if you use the one you've opened before "job is done", and write it with incremented version, db exception should do the trick. Am I correct?

[marinkobabic]

Correct. If NES would keep the stream opened during the whole UnitOfWork, then we would get the db exception like expected by you previously.

Actually during the read NES opens and closes the stream. Then, when NES is going to write, it opens the stream again and then it writes and closes the stream.

[lees-oz]

Clear, and what is the reason of not keeping it open? What kind of trade-off is it?

[marinkobabic]

Did not analyze what consequences this would have. What are the drawbacks of the actual solution etc.

Any suggestions and pull requests are welcome.