set_count_on_hand should use lock

[bbuchalter]

This is the same pattern that was followed for adjust_count_on_hand to
avoid deadlocks.

Reopens #1101 to see if this will now pass on master.

[bbuchalter]

This new PR does cause the same test failure as #1101:

  1) Spree::StockItem#after_save inventory_cache_threshold is set beginning above threshold count on hand stays above threshold
     Failure/Error:
       expect do
         subject.set_count_on_hand(8)
       end.not_to change { subject.variant.updated_at }
     
       expected `subject.variant.updated_at` not to have changed, but did change from 2017-06-12 08:10:14.927710593 +0000 to 2017-06-12 08:10:14.927710000 +0000
     # ./spec/models/spree/stock_item_spec.rb:217:in `block (5 levels) in <top (required)>'

[bbuchalter]

Like @peterberkenbosch originally reported, it's passing locally.

[bbuchalter]

Also worth noting this raises a new depreciation warning:

DEPRECATION WARNING: Locking a record with unpersisted changes is deprecated and will raise an exception in Rails 5.2. Use `save` to persist the changes, or `reload` to discard them explicitly. (called from set_count_on_hand at /Users/brian/workspace/solidus_master/core/app/models/spree/stock_item.rb:48)

[bbuchalter]

There are now a few of these warnings in core. See rails/rails#25873 for details.

core/spec/models/spree/product_spec.rb:373
core/spec/models/spree/order_merger_spec.rb:150
core/spec/models/spree/variant_spec.rb:152

I am unsure how to proceed because I believe we need to address the deprecation warnings generally and I cannot reproduce the CI test failure locally, even when running the full core suite with the same seed as CI.

[tvdeyen]

The local dummy test app uses sqlite as database, while CircleCI tests with posgresql.

[bbuchalter]

The local dummy test app uses sqlite as database, while CircleCI tests with posgresql.

Could you remind me how to switch locally?

Also of note, I do not get the depreciation notice locally, which may suggest something else is dirtying the stock item's state outside the spec?

[tvdeyen]

Could you remind me how to switch locally?

Not possible through rake test_app as this does not allow to pass options to the dummy generator. But you should be able to invoke the rails g spree:dummy --database=postgresql independently. Not able to test this on my own right now, sorry 😬 But worth a try

[bbuchalter]

I was able to follow the README and use DB=postgresql bundle exec rake test_app.

Still passes locally against latest master with same seed as CI.

[bbuchalter]

Also of note, I do not get the depreciation notice locally, which may suggest something else is dirtying the stock item's state outside the spec?

I wanted to clarify this a bit. After adding this change, there are new deprecation warnings when the full core spec suite runs. However it's not stock_item_spec.rb that's causing the warnings as is the case with all the others; it's actually coming from core/spec/models/spree/stock/packer_spec.rb.

[mtomov]

I was actually under the impression that with_lock would add lock, thereby increasing deadlocks 🤔

I'm actually thinking that this process_backorders(count_on_hand - count_on_hand_was) is rather meant to be in a background job of sorts, as nothing should immediately depend on its result and it's logic could potentially get rather heavy. I remember it causing a lot of noise (trouble) in my previous company.

[bbuchalter]

Upon further reflection, I don't believe I can stand behind this PR. When it comes to addressing DB lock issues, I want to be confident I can reproduce and eliminate the deadlocks via a load test. Haphazardly adding locks, as @mtomov rightly suggests, is not the right path.

Ideas for new issues:

• Add automated load testing for the API to identify deadlocks and performance regressions
• Address DEPRECATION WARNING: Locking a record with unpersisted changes is deprecated and will raise an exception in Rails 5.2. that were seen in other parts of the test suite.

Please let me know if there is interest in the issues above.