InMemoryAdapter runs easily into race conditions

[mentero]

Hi, we ran into a problem with using InMemoryAdapter today. I prepared an example repository that reproduces the problem.

https://github.com/mentero/ruby_event_store_test

Long story short. We have daily workers running at night and we wanted to publish some events from them. We have similar setup to this one. The problem is there are many sidekiq workers running at the same time and we got flooded by RubyEventStore::WrongExpectedEventVersion quickly.

We have investigated the problem and it seems like a classic race condition caused by expected_version being calculated outside of the mutex. It is very easy to run into a scenario where two workers calculate the same version number before entering synchronized block.

I don't know why InMemoryAdapter was chosen over ActiveRecord one, but nevertheless this behaviour should probably be considered a bug as it makes InMemoryAdapter very brittle.

[paneq]

@mentero Thanks for your ticket!

a) InMemoryAdapter is not documented right now
b) We've never expected it to be used in production, I believe.
c) what RES version are we talking about?

We will definitely need to think about, but I cannot promise solving this soon.

[mentero]

@paneq

We have 0.20 in our app but I reproduced that on the 0.25 in the repository linked above

[paneq]

Related: *

rails_event_store/ruby_event_store/lib/ruby_event_store/in_memory_repository.rb

Line 99 in 5baff28

@mutex.synchronize do

• d1f52b8#diff-ef17629f8f1d7edd404efaa4b47a6fb2R9

[paneq]

Related:

module RubyEventStore
  RSpec.describe InMemoryRepository do
    # There is no way to use in-memory adapter in a
    # lock-free, unlimited concurrency way
    let(:test_race_conditions_any)   { false }
    let(:test_race_conditions_auto)  { true }
    let(:test_expected_version_auto) { true }
    let(:test_link_events_to_stream) { true }

https://github.com/RailsEventStore/rails_event_store/blob/master/ruby_event_store/spec/in_memory_repository_spec.rb

[paneq]

@mentero Also, we would be happy if you fill out our poll about RES usage: https://goo.gl/forms/ecNpJsL5qM2M8Ls93 if you haven't yet :) Thank you and have a great day!

[paneq]

but I reproduced that on the 0.25 in the repository linked above

:(

[mentero]

The problem is

rails_event_store/ruby_event_store/lib/ruby_event_store/in_memory_repository.rb

Line 81 in 5baff28

stream.size - 1

happening before this

rails_event_store/ruby_event_store/lib/ruby_event_store/in_memory_repository.rb

Line 99 in 5baff28

@mutex.synchronize do

which causes this to fail

rails_event_store/ruby_event_store/lib/ruby_event_store/in_memory_repository.rb

Line 105 in 5baff28

raise WrongExpectedEventVersion unless (stream.size - 1).equal?(expected_version)

[paneq]

@mentero indeed.

[mentero]

I have tried a naive fix and moved calculating expected_version inside the mutex but it broke tests that expect some failures when :auto is used outside of a lock

[paneq]

@mentero I just checked and right now There is no way to use in-memory adapter in a lock-free, unlimited concurrency way. It was not designed like that. Right now :any version behaves as :auto ( http://railseventstore.org/docs/expected_version/ ) in InMemoryRepository.

We could try to fix it so the InMemoryRepository would work according to the documentation :)

I think it would require calling stream.size - 1 inside the lock for :any and outside for :auto.

[paneq]

@mentero Is that application ever reading those events? Is it ever clearing the always growing in memory list of events?

If it is never reading, then you it means you are using RES just as pub/sub without intended persistence. In that case you could substitute a Repository which does nothing and it would work better for the usecase :) I think it might be even beneficial to have such NullRepository which does not save as part of RES.

[siemakuba]

@paneq this is some weird mind-reading happening around here...
I've just pushed a Pull Request with exactly this kind of changes - an object utilizing the NullObject pattern that is passed as a repository to the RES Client.

It's dead simple, but first tests show that It'd work just fine.
I'd be happy to prepare a PR for RES introducing this solution.

[siemakuba]

And yes, your're absolutely right with:

If it is never reading, then it means you are using RES just as pub/sub without intended persistence.

;)

[paneq]

Siema Kuba :)

I'd be happy to prepare a PR for RES introducing this solution.

yes, please :)

[paneq]

@mentero @siemakuba fixed on master, to be included in next release. Thank you for your contribution.