16470: Adds an explicit reload before every pending with_lock call.

Previous versions of rails did an implicit reload when calling with_lock,
but from 5.2 calling with_lock with unpersisted changes will raise an
exception.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas@di-pentima.com.ar>

diff --git a/services/api/app/controllers/arvados/v1/containers_controller.rb b/services/api/app/controllers/arvados/v1/containers_controller.rb
index 041f55947229d4d6d802de348e5561dce313c96e..b2324a57124cf4fb807820641694de8f53bfb51e 100644 (file)
--- a/services/api/app/controllers/arvados/v1/containers_controller.rb
+++ b/services/api/app/controllers/arvados/v1/containers_controller.rb
@@ -29,7 +29,7 @@ class Arvados::V1::ContainersController < ApplicationController
   end
 
   def update
   end
 
   def update

-    @object.with_lock do
+    @object.reload.with_lock do

       super
     end
   end
       super
     end
   end

diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index adfbf6042f22a433275bf016e9ff6d4989ea9978..16f8cd798fe574e5e758ceaa600c8367beaea13a 100644 (file)
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -339,7 +339,7 @@ class Container < ArvadosModel
   end
 
   def lock
   end
 
   def lock

-    self.with_lock do
+    self.reload.with_lock do

       if self.state != Queued
         raise LockFailedError.new("cannot lock when #{self.state}")
       end
       if self.state != Queued
         raise LockFailedError.new("cannot lock when #{self.state}")
       end


@@ -357,7 +357,7 @@ class Container < ArvadosModel
   end
 
   def unlock
   end
 
   def unlock

-    self.with_lock do
+    self.reload.with_lock do

       if self.state != Locked
         raise InvalidStateTransitionError.new("cannot unlock when #{self.state}")
       end
       if self.state != Locked
         raise InvalidStateTransitionError.new("cannot unlock when #{self.state}")
       end


@@ -654,7 +654,7 @@ class Container < ArvadosModel
     # This container is finished so finalize any associated container requests
     # that are associated with this container.
     if saved_change_to_state? and self.final?
     # This container is finished so finalize any associated container requests
     # that are associated with this container.
     if saved_change_to_state? and self.final?

-      # These get wiped out by with_lock (which reloads the record),
+      # These get wiped out (with_lock requires to explicitly reload the record),

       # so record them now in case we need to schedule a retry.
       prev_secret_mounts = secret_mounts_before_last_save
       prev_runtime_token = runtime_token_before_last_save
       # so record them now in case we need to schedule a retry.
       prev_secret_mounts = secret_mounts_before_last_save
       prev_runtime_token = runtime_token_before_last_save


@@ -665,7 +665,7 @@ class Container < ArvadosModel
       # transaction finishes.  This ensure that concurrent container
       # requests that try to reuse this container are finalized (on
       # Complete) or don't reuse it (on Cancelled).
       # transaction finishes.  This ensure that concurrent container
       # requests that try to reuse this container are finalized (on
       # Complete) or don't reuse it (on Cancelled).

-      self.with_lock do
+      self.reload.with_lock do

         act_as_system_user do
           if self.state == Cancelled
             retryable_requests = ContainerRequest.where("container_uuid = ? and priority > 0 and state = 'Committed' and container_count < container_count_max", uuid)
         act_as_system_user do
           if self.state == Cancelled
             retryable_requests = ContainerRequest.where("container_uuid = ? and priority > 0 and state = 'Committed' and container_count < container_count_max", uuid)


@@ -690,7 +690,7 @@ class Container < ArvadosModel
             }
             c = Container.create! c_attrs
             retryable_requests.each do |cr|
             }
             c = Container.create! c_attrs
             retryable_requests.each do |cr|

-              cr.with_lock do
+              cr.reload.with_lock do

                 leave_modified_by_user_alone do
                   # Use row locking because this increments container_count
                   cr.container_uuid = c.uuid
                 leave_modified_by_user_alone do
                   # Use row locking because this increments container_count
                   cr.container_uuid = c.uuid

diff --git a/services/api/app/models/job.rb b/services/api/app/models/job.rb
index 37e5f455dffe73b61c783afc219913a6daf8313f..31179f0666b040b68bab68f131fea7d1e10c98ec 100644 (file)
--- a/services/api/app/models/job.rb
+++ b/services/api/app/models/job.rb
@@ -136,7 +136,7 @@ class Job < ArvadosModel
   end
 
   def lock locked_by_uuid
   end
 
   def lock locked_by_uuid

-    with_lock do
+    reload.with_lock do

       unless self.state == Queued and self.is_locked_by_uuid.nil?
         raise AlreadyLockedError
       end
       unless self.state == Queued and self.is_locked_by_uuid.nil?
         raise AlreadyLockedError
       end

diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index addea83062404b84baf1911d6b81a262d582ce05..dbfdae95acbd544c3672059c426612825e6d8aee 100644 (file)
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -515,7 +515,7 @@ class CollectionTest < ActiveSupport::TestCase
       c2.description = 'foo collection'
       c1.save!
       assert_equal 1, c2.version
       c2.description = 'foo collection'
       c1.save!
       assert_equal 1, c2.version

-      # with_lock forces a reload, so this shouldn't produce an unique violation error
+      # with_lock requires a reload, so this shouldn't produce an unique violation error

       c2.save!
       assert_equal 3, c2.version
       assert_equal 'foo collection', c2.description
       c2.save!
       assert_equal 3, c2.version
       assert_equal 'foo collection', c2.description