fixes #1705 by locking node while Assigning rel #1706
Conversation
| @@ -62,12 +63,13 @@ def add_rels(node_or_nodes, original_ids) | |||
| end | |||
|
|
|||
| def delete_rels_for_nodes(original_ids, new_ids) | |||
| ids = original_ids.select { |id| !new_ids.include?(id) } | |||
There was a problem hiding this comment.
using - is faster compared to using select loop and then checking with include? again
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1706 +/- ##
==========================================
- Coverage 93.43% 93.26% -0.18%
==========================================
Files 128 128
Lines 5909 5923 +14
==========================================
+ Hits 5521 5524 +3
- Misses 388 399 +11 ☔ View full report in Codecov by Sentry. |
lib/active_graph/transactions.rb
Outdated
| @@ -28,6 +28,10 @@ def read_transaction(**config, &block) | |||
|
|
|||
| alias transaction write_transaction | |||
|
|
|||
| def lock_node(node) | |||
| node.as(:n).query.remove('n._LOCK_').exec if tx&.open? || explicit_session&.open? | |||
There was a problem hiding this comment.
it would be good to ensure that your specs fail with this code:
| node.as(:n).query.remove('n._LOCK_').exec if tx&.open? || explicit_session&.open? | |
| node.as(:n).query.exec if tx&.open? || explicit_session&.open? |
There was a problem hiding this comment.
Where is the tx boundary set in specs? What tx do new threads use?
There was a problem hiding this comment.
The specs are too complex to be confident without failing version.
There was a problem hiding this comment.
with disabling my code in lock_node I get following error so we now have a simpler specs with proper failure without fix.
1) ActiveGraph::Relationship::Persistence::QueryFactory#build! non-subclassed concurrent update does not create duplicate has_one relationship
Failure/Error: expect(ActiveGraph::Base.query("MATCH (node2:`ToClass`)<-[rel1:`HAS_REL`]-(from_class:`FromClass`) return from_class").to_a.size).to eq(1)
expected: 1
got: 19
(compared using ==)
# ./spec/e2e/relationship/persistence/query_factory_spec.rb:126:in `block (5 levels) in <top (required)>'
```
There was a problem hiding this comment.
@klobuczek every thread has unique tx, I checked their object_id.
lib/active_graph/transactions.rb
Outdated
| @@ -28,6 +28,10 @@ def read_transaction(**config, &block) | |||
|
|
|||
| alias transaction write_transaction | |||
|
|
|||
| def lock_node(node) | |||
| node.as(:n).query.remove('n._AGLOCK_').exec if tx&.open? || explicit_session&.open? | |||
There was a problem hiding this comment.
why the condition? No harm would happen if the condition is not met.
| before do | ||
| allow_any_instance_of(ActiveGraph::Node::Query::QueryProxy).to receive(:replace_with).and_wrap_original do |original, *args| | ||
| $concurrency_queue << 'ready' | ||
| Thread.stop |
There was a problem hiding this comment.
I think the right thing would be to stop at delete_rels_for_nodes after the rels are read so you can force failure with just 2 threads not 100. And then after adding lock see it passing.
| context 'concurrent update' do | ||
| before do | ||
| allow_any_instance_of(ActiveGraph::Node::Query::QueryProxy).to receive(:replace_with).and_wrap_original do |original, *args| | ||
| $concurrency_queue << 'ready' | ||
| Thread.stop | ||
| original.call(*args) | ||
| end | ||
| end | ||
| after { $concurrency_queue = nil } | ||
| let!(:from_node) { FromClass.create(name: 'foo') } | ||
| let!(:to_node) { ToClass.create(name: 'bar') } | ||
|
|
||
| it 'does not create duplicate has_one relationship' do | ||
| count = 100 | ||
| $concurrency_queue = Thread::Queue.new | ||
| threads = count.times.map { Thread.new { to_node.update(from_class: from_node) } } | ||
| sleep(0.1) until $concurrency_queue.size == count | ||
| $concurrency_queue.clear | ||
| threads.each(&:run) | ||
| threads.each(&:join) | ||
| expect(ActiveGraph::Base.query("MATCH (node2:`ToClass`)<-[rel1:`HAS_REL`]-(from_class:`FromClass`) return from_class").to_a.size).to eq(1) | ||
| end | ||
| end |
There was a problem hiding this comment.
I am not thrilled about this spec. sleep, 100 threads, no guarantee of failure without lock, low level api. I think this would be possible async.
|
closing as already merged to zeitwerk branch |
Fixes #1705
This pull introduces/changes: