Review comments

Author: Michael Kipper

README.md

@@ -187,7 +187,7 @@ Per the above example, you no longer need to care about the number of tickets. R
 
 In this case, we'd allow 49% of the workers on a particular host to connect to this redis resource.
 
-In particular, 1 worker = 1 ticket (due to `ceil`), 2 workers = 2 tickets (due to `min_tickets`), 4 workers = 2 tickets, 16 workers = 8 tickets, 100 workers = 49 tickets.
+In particular, 1 worker = 1 ticket (due to `ceil`), 2 workers = 2 tickets (due to `min_tickets`), 4 workers = 2 tickets (due to `ceil`), 100 workers = 49 tickets.
 
 **Note**:
 

ext/semian/resource.c

@@ -4,14 +4,17 @@ static VALUE
 cleanup_semian_resource_acquire(VALUE self);
 
 static void
-check_tickets_xor_quota_arg(VALUE tickets, VALUE quota);
+check_tickets_xor_quota_arg(VALUE tickets, VALUE min_tickets, VALUE quota);
 
 static double
 check_quota_arg(VALUE quota);
 
 static int
 check_tickets_arg(VALUE tickets);
 
+static int
+check_min_tickets_arg(VALUE min_tickets);
+
 static long
 check_permissions_arg(VALUE permissions);
 
@@ -203,13 +206,13 @@ VALUE
 semian_resource_initialize(VALUE self, VALUE id, VALUE tickets, VALUE quota, VALUE permissions, VALUE default_timeout, VALUE min_tickets)
 {
   // Check and cast arguments
-  check_tickets_xor_quota_arg(tickets, quota);
+  check_tickets_xor_quota_arg(tickets, min_tickets, quota);
   double c_quota = check_quota_arg(quota);
   int c_tickets = check_tickets_arg(tickets);
   long c_permissions = check_permissions_arg(permissions);
   const char *c_id_str = check_id_arg(id);
   double c_timeout = check_default_timeout_arg(default_timeout);
-  int c_min_tickets = check_tickets_arg(min_tickets);
+  int c_min_tickets = check_min_tickets_arg(min_tickets);
 
   // Build semian resource structure
   semian_resource_t *res = NULL;
@@ -254,10 +257,22 @@ check_permissions_arg(VALUE permissions)
 }
 
 static void
-check_tickets_xor_quota_arg(VALUE tickets, VALUE quota)
+check_tickets_xor_quota_arg(VALUE tickets, VALUE min_tickets, VALUE quota)
 {
-  if ((TYPE(tickets) == T_NIL && TYPE(quota) == T_NIL) ||(TYPE(tickets) != T_NIL && TYPE(quota) != T_NIL)){
-    rb_raise(rb_eArgError, "Must pass exactly one of ticket or quota");
+  const char *msg = "Must pass exactly one of ticket or quota/min_tickets";
+  if (TYPE(quota) != T_NIL) {
+    if (TYPE(tickets) != T_NIL) {
+      dprintf("FOO");
+      rb_raise(rb_eArgError, msg);
+    }
+  } else if (TYPE(tickets) != T_NIL) {
+    if (TYPE(quota) != T_NIL || TYPE(min_tickets) != T_NIL) {
+      dprintf("FOO");
+      rb_raise(rb_eArgError, msg);
+    }
+  } else {
+      dprintf("FOO");
+    rb_raise(rb_eArgError, msg);
   }
 }
 
@@ -303,6 +318,32 @@ check_tickets_arg(VALUE tickets)
   return c_tickets;
 }
 
+static int
+check_min_tickets_arg(VALUE min_tickets)
+{
+  int retval = -1;
+
+  switch (rb_type(min_tickets)) {
+  case T_NIL:
+  case T_UNDEF:
+    return -1;
+  case T_FLOAT:
+    rb_warn("semian min_tickets value %f is a float, converting to fixnum", RFLOAT_VALUE(min_tickets));
+    retval = (int) RFLOAT_VALUE(min_tickets);
+    break;
+  case T_FIXNUM:
+    retval = FIX2LONG(min_tickets); break;
+  default:
+    retval = -1; break;
+  }
+
+  if (retval <= 0 || retval > system_max_semaphore_count) {
+    rb_raise(rb_eArgError, "max_tickets must be in range [1,%d)", system_max_semaphore_count);
+  }
+
+  return retval;
+}
+
 static const char*
 check_id_arg(VALUE id)
 {

ext/semian/tickets.c

@@ -80,10 +80,10 @@ max(const int a, const int b)
 }
 
 static int
-calculate_quota_tickets (int sem_id, double quota, int min_tickets)
+calculate_quota_tickets(int sem_id, double quota, int min_tickets)
 {
   int workers = get_sem_val(sem_id, SI_SEM_REGISTERED_WORKERS);
   int tickets = (int) ceil(workers * quota);
-  dprintf("Calculating quota tickets - sem_id:%d quota:%0.2f%% workers:%d min_tickets:%d tickets:%d", sem_id, quota, workers, min_tickets, tickets);
+  dprintf("Calculating quota tickets - sem_id:%d quota:%0.2f%% workers:%d min_tickets:%d tickets:%d", sem_id, quota * 100.0, workers, min_tickets, tickets);
   return min_tickets > 0 ? min(workers, max(tickets, min_tickets)) : tickets;
 }

ext/semian/util.h

@@ -5,7 +5,7 @@
 #include <stdio.h>
 #include <time.h>
 
-#if defined(DEBUG) || defined(SEMIAN_DEBUG)
+#ifdef DEBUG
 #  define DEBUG_TEST 1
 #else
 #  define DEBUG_TEST 0

lib/semian/resource.rb

@@ -9,7 +9,7 @@ def instance(*args)
       end
     end
 
-    def initialize(name, tickets: nil, quota: nil, permissions: 0660, timeout: 0, min_tickets: 1)
+    def initialize(name, tickets: nil, quota: nil, permissions: 0660, timeout: 0, min_tickets: nil)
       if Semian.semaphores_enabled?
         initialize_semaphore(name, tickets, quota, permissions, timeout, min_tickets) if respond_to?(:initialize_semaphore)
       else

test/resource_test.rb

@@ -506,6 +506,14 @@ def test_min_tickets
     assert_equal(8, resource.tickets)
   end
 
+  def test_min_tickets_float
+    expected_warning = /semian min_tickets value 2\.000000 is a float, converting to fixnum/
+    with_fake_std_error(warn_message: expected_warning) do
+      id = Time.now.strftime('%H:%M:%S.%N')
+      Semian::Resource.new(id, quota: 0.49, timeout: 0.1, min_tickets: 2.0)
+    end
+  end
+
   def test_min_tickets_nil
     id = Time.now.strftime('%H:%M:%S.%N')
     resource = Semian::Resource.new(id, quota: 0.49, timeout: 0.1, min_tickets: nil)
@@ -522,16 +530,9 @@ def test_min_tickets_nil
 
   def test_min_tickets_zero
     id = Time.now.strftime('%H:%M:%S.%N')
-    resource = Semian::Resource.new(id, quota: 0.49, timeout: 0.1, min_tickets: 0)
-    assert_equal(1, resource.tickets)
-    fork_workers(resource: id, count: 1, quota: 0.49, min_tickets: 0, timeout: 0.1, wait_for_timeout: true)
-    assert_equal(1, resource.tickets)
-    fork_workers(resource: id, count: 1, quota: 0.49, min_tickets: 0, timeout: 0.1, wait_for_timeout: true)
-    assert_equal(2, resource.tickets)
-    fork_workers(resource: id, count: 1, quota: 0.49, min_tickets: 0, timeout: 0.1, wait_for_timeout: true)
-    assert_equal(2, resource.tickets)
-    fork_workers(resource: id, count: 12, quota: 0.49, min_tickets: 0, timeout: 0.1, wait_for_timeout: true)
-    assert_equal(8, resource.tickets)
+    assert_raises ArgumentError do
+      Semian::Resource.new(id, quota: 0.49, timeout: 0.1, min_tickets: 0)
+    end
   end
 
   def test_min_tickets_negative
@@ -541,6 +542,13 @@ def test_min_tickets_negative
     end
   end
 
+  def test_min_tickets_out_of_range
+    id = Time.now.strftime('%H:%M:%S.%N')
+    assert_raises ArgumentError do
+      Semian::Resource.new(id, quota: 0.49, timeout: 0.1, min_tickets: 32768)
+    end
+  end
+
   def create_resource(*args)
     @resources ||= []
     resource = Semian::Resource.new(*args)

test/semian_test.rb

@@ -35,7 +35,7 @@ def test_register_with_bulkhead_missing_options
         circuit_breaker: false,
       )
     end
-    assert_equal exception.message, "Must pass exactly one of ticket or quota"
+    assert_equal exception.message, "Must pass exactly one of ticket or quota/min_tickets"
   end
 
   def test_unsuported_constants