yegor256 / factbase

In-memory database of facts (records with attributes) with a predicative searching facility
https://rubygems.org/gems/factbase
MIT License
5 stars 3 forks source link

there are no tests for thread-safety #98

Closed yegor256 closed 2 months ago

yegor256 commented 4 months ago

We claim that Factbase class is thread-safe, but there are no tests that confirm this. Let's use threads Gem, as explained here, to create such tests.

Suban05 commented 2 months ago

@yegor256 do I understand correctly, that this class is really thread-safe, but we don't have any tests to approve this fact?

It's just that if I open the code of the class, then I see this comment: https://github.com/yegor256/factbase/blob/2e2dae296e2273aefee245240c82865122780df2/lib/factbase.rb#L77

yegor256 commented 2 months ago

@Suban05 I don't think it's thread safe. If you do fb.insert.foo = 42 from 100 different threads at the same time, you won't see 100 facts in fb. Most probably, there will be fewer.

Suban05 commented 2 months ago

@yegor256 if I understood your described case correctly, this test passes:

def test_concurrent_inserts
  fb = Factbase.new
  Threads.new(100).assert do
    fb.insert.foo = 42
  end
  assert_equal(100, fb.size)
end

that's very good

yegor256 commented 2 months ago

@Suban05 well, maybe we are thread-safe already indeed. Let's add the test. Maybe, let's make it a bit more complicated, with more fb-manipulations inside each thread.

Suban05 commented 2 months ago

@yegor256 of course, I will do it assign me, please)

yegor256 commented 2 months ago

@Suban05 go ahead, please

Suban05 commented 2 months ago

@yegor256 this test fails:

def test_concurrent_queries
  fb = Factbase.new
  Threads.new(100).assert do |i|
    fact = fb.insert
    fact.thread_id = i
    fact.value = i * 10
  end
  Threads.new(100).assert do
    results = fb.query('(exists thread_id)').each.to_a
    assert_equal(100, results.size)

    thread_ids = results.map { |fact| fact.thread_id }
    assert_equal((0..99).to_a, thread_ids.sort)
  end
end

When I execute a query, all the facts have thread_id = 0.

But this test is OK:

def test_concurrent_queries
  fb = Factbase.new
  100.times do |i|
    fact = fb.insert
    fact.thread_id = i
    fact.value = i * 10
  end
  Threads.new(100) do
    results = fb.query('(exists thread_id)').each.to_a
    assert_equal(100, results.size)

    thread_ids = results.map { |fact| fact.thread_id }
    assert_equal((0..99).to_a, thread_ids.sort)
  end
end

Do I do something incorrectly or is it a mistake?

yegor256 commented 2 months ago

@Suban05 looks weird. I don't know why the test fails. You wrote it correctly.

Suban05 commented 2 months ago

@yegor256 sometimes this test also fails:


def test_concurrent_transactions_successful
  fb = Factbase.new
  Threads.new(100).assert do |i|
    fb.txn do |fbt|
      fact = fbt.insert
      fact.thread_id = i
      fact.value = i * 10
    end
  end
  facts = fb.query('(exists thread_id)').each.to_a
  assert_equal(100, facts.size)
  facts.each do |fact|
    assert_equal(fact.value, fact.thread_id * 10)
  end
end

it's very rare, but I caught the situation, when facts.size equalled 95, instead of 100

What will we do? We can't solve this problem, because we really are not thread-safe

yegor256 commented 2 months ago

@Suban05 we must be thread-safe. You can add a test to the code base, but skip it, making a puzzle inside (read about PDD). Thus, you will earn some points for the PR and we'll continue from the puzzle.

Suban05 commented 2 months ago

@yegor256 if I rewrite this test (and other similar ones):

def test_concurrent_queries
  fb = Factbase.new
  Threads.new(100).assert do |i|
    fact = fb.insert
    fact.thread_id = i
    fact.value = i * 10
  end
  Threads.new(100).assert do
    results = fb.query('(exists thread_id)').each.to_a
    assert_equal(100, results.size)

    thread_ids = results.map(&:thread_id)
    assert_equal((0..99).to_a, thread_ids.sort)
  end
end

like this:

def test_concurrent_queries
  fb = Factbase.new
  i = 0 # count manually
  Threads.new(100).assert do
    fact = fb.insert
    fact.thread_id = i
    fact.value = i * 10
    i += 1
  end
  Threads.new(100).assert do
    results = fb.query('(exists thread_id)').each.to_a
    assert_equal(100, results.size)

    thread_ids = results.map(&:thread_id)
    assert_equal((0..99).to_a, thread_ids.sort)
  end
end

the second test (and all the other tests who had problems) is OK. I can't figure out what the problem is.

I wrote the simple code like this:

arr = []
Threads.new(5).assert do |i|
  arr << i
end

And an array has only items that are equal to 0.

but this code outputs 0, 1, 2, 3, 4:

Threads.new(5).assert do |i|
  puts i
end

Is it a problem with the threads gem, or maybe I'm using it incorrectly?

yegor256 commented 2 months ago

@Suban05 this code is NOT thread-safe:

arr = []
Threads.new(5).assert do |i|
  arr << i
end

All five threads are trying to append a number to the array, but only one thread "wins" the race and adds the number, for example 0.

0pdd commented 2 months ago

@yegor256 3 puzzles #115, #116, #117 are still not solved.