From 36892a1cbbb1019d7394a654be4500d1003fc88b Mon Sep 17 00:00:00 2001
From: Eduardo Martin Rojo <emr@sanger.ac.uk>
Date: Mon, 23 Oct 2023 16:55:05 +0100
Subject: [PATCH] Added module to fix race condition when running test in local
 on obtaining plate barcodes

---
 app/models/plate_barcode.rb                   | 13 ++++-
 lib/dev/plate_barcode/cache_barcodes.rb       | 51 +++++++++++++++++++
 .../dev/plate_barcode/cache_barcodes_spec.rb  | 46 +++++++++++++++++
 3 files changed, 108 insertions(+), 2 deletions(-)
 create mode 100644 lib/dev/plate_barcode/cache_barcodes.rb
 create mode 100644 spec/lib/dev/plate_barcode/cache_barcodes_spec.rb

diff --git a/app/models/plate_barcode.rb b/app/models/plate_barcode.rb
index d2e5d21e4f..6aa1a8e7cd 100644
--- a/app/models/plate_barcode.rb
+++ b/app/models/plate_barcode.rb
@@ -103,11 +103,20 @@ def self._connection_scope(url, data = nil)
   if Rails.env.development?
     # If we don't want a test dependency on baracoda we need to mock barcodes and child barcodes
 
+    #
+    # When in development we were receiving concurrent requests for a barcode
+    # we were experiencing a race condition where both requests were obtaining the same barcode.
+    # To avoid it we will cache the barcodes and perform retry until unique barcode is
+    # obtained
+    extend Dev::PlateBarcode::CacheBarcodes
+
     def self.create_barcode
       # We should use a different prefix for local so that you can switch between using baracoda
       # locally and there will not be clashes
-      current_num = Barcode.sequencescape22.order(id: :desc).first&.number || 9000
-      Barcode.build_sequencescape22({ barcode: "#{prefix}-#{current_num + 1}" })
+
+      # We cache the last barcode so we don't get race conditions on generating barcodes
+      # when requests appear at the same time
+      Barcode.build_sequencescape22({ barcode: "#{prefix}-#{dev_cache_get_next_barcode}" })
     end
 
     def self.create_child_barcodes(parent_barcode, count = 1)
diff --git a/lib/dev/plate_barcode/cache_barcodes.rb b/lib/dev/plate_barcode/cache_barcodes.rb
new file mode 100644
index 0000000000..9cc6a81219
--- /dev/null
+++ b/lib/dev/plate_barcode/cache_barcodes.rb
@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+module Dev
+  module PlateBarcode
+    # When in development mode we were receiving concurrent requests for creating a barcode
+    # which produced a race condition where all concurrent requests were obtaining the same barcode.
+    # To avoid it this module implements a cache of barcodes so it should be able to support
+    # up to MAX_SIZE_CACHE concurrent requests which we expect should be enough for
+    # development.
+    module CacheBarcodes
+      # Obtains a new barcode and if the obtained barcode was obtained before (if it was cached)
+      # it will retry and obtain a new barcode
+      def dev_cache_get_next_barcode
+        pos = 1
+        next_barcode = nil
+        loop do
+          next_barcode = (Barcode.sequencescape22.order(id: :desc).first&.number || 9000) + pos
+          pos += 1
+          break unless barcode_in_cache?(next_barcode)
+        end
+        cache_barcode(next_barcode)
+        next_barcode
+      end
+
+      MAX_SIZE_CACHE = 100
+
+      # Cache stored in a class property (in PlateBarcode)
+      def data_cache
+        @data_cache ||= []
+      end
+
+      def reset_cache
+        @data_cache = []
+      end
+
+      # Avoids the cache to grow out of the cache limits
+      def resize_cache
+        @data_cache = data_cache.drop(1) if data_cache.length >= MAX_SIZE_CACHE
+      end
+
+      def cache_barcode(barcode)
+        resize_cache
+
+        data_cache.push(barcode)
+      end
+
+      def barcode_in_cache?(barcode)
+        data_cache.include?(barcode)
+      end
+    end
+  end
+end
diff --git a/spec/lib/dev/plate_barcode/cache_barcodes_spec.rb b/spec/lib/dev/plate_barcode/cache_barcodes_spec.rb
new file mode 100644
index 0000000000..7a1cac2e9f
--- /dev/null
+++ b/spec/lib/dev/plate_barcode/cache_barcodes_spec.rb
@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+require 'rails_helper'
+
+RSpec.describe Dev::PlateBarcode::CacheBarcodes do
+  describe '#dev_cache_get_next_barcode' do
+    let(:my_class) { Class.new { extend Dev::PlateBarcode::CacheBarcodes } }
+    let(:instance) { my_class.new }
+
+    context 'when getting new barcodes' do
+      before { my_class.reset_cache }
+
+      it 'gets different barcodes on each call' do
+        expect(my_class.dev_cache_get_next_barcode).to eq(9001)
+        expect(my_class.dev_cache_get_next_barcode).to eq(9002)
+        expect(my_class.dev_cache_get_next_barcode).to eq(9003)
+      end
+    end
+
+    context 'with the cache management' do
+      before { my_class.reset_cache }
+
+      it 'cache grows with every new barcode' do
+        expect { my_class.dev_cache_get_next_barcode }.to change { my_class.data_cache.length }.by(1)
+        expect do
+          my_class.dev_cache_get_next_barcode
+          my_class.dev_cache_get_next_barcode
+          my_class.dev_cache_get_next_barcode
+        end.to change { my_class.data_cache.length }.by(3)
+      end
+
+      it 'does not grow over the max size' do
+        pos = 0
+        while pos < Dev::PlateBarcode::CacheBarcodes::MAX_SIZE_CACHE
+          my_class.dev_cache_get_next_barcode
+          pos += 1
+        end
+
+        expect do
+          my_class.dev_cache_get_next_barcode
+          my_class.dev_cache_get_next_barcode
+          my_class.dev_cache_get_next_barcode
+        end.not_to(change { my_class.data_cache.length })
+      end
+    end
+  end
+end