Review fixes

Author: Michael Kipper

ext/semian/simple_integer.c

@@ -8,16 +8,12 @@
 
 static const rb_data_type_t semian_simple_integer_type;
 
-static int* get_value(VALUE self) {
-  const char* name = to_s(rb_iv_get(self, "@name"));
-  if (name == NULL) {
-    rb_raise(rb_eArgError, "could not get object name");
-  }
-
-  key_t key = generate_key(name);
+static semian_simple_integer_shared_t* get_value(VALUE self) {
+  semian_simple_integer_t *res;
+  TypedData_Get_Struct(self, semian_simple_integer_t, &semian_simple_integer_type, res);
 
   const int permissions = 0664;
-  int shmid = shmget(key, sizeof(int), IPC_CREAT | permissions);
+  int shmid = shmget(res->key, sizeof(semian_simple_integer_shared_t), IPC_CREAT | permissions);
   if (shmid == -1) {
     rb_raise(rb_eArgError, "could not create shared memory (%s)", strerror(errno));
   }
@@ -27,7 +23,7 @@ static int* get_value(VALUE self) {
     rb_raise(rb_eArgError, "could not get shared memory (%s)", strerror(errno));
   }
 
-  return (int*)val;
+  return (semian_simple_integer_shared_t*)val;
 }
 
 void Init_SimpleInteger()
@@ -37,8 +33,8 @@ void Init_SimpleInteger()
   VALUE cSimpleInteger = rb_const_get(cSimple, rb_intern("Integer"));
 
   rb_define_alloc_func(cSimpleInteger, semian_simple_integer_alloc);
-  rb_define_method(cSimpleInteger, "initialize_simple_integer", semian_simple_integer_initialize, 0);
-  rb_define_method(cSimpleInteger, "increment", semian_simple_integer_increment, 1);
+  rb_define_method(cSimpleInteger, "initialize_simple_integer", semian_simple_integer_initialize, 1);
+  rb_define_method(cSimpleInteger, "increment", semian_simple_integer_increment, -1);
   rb_define_method(cSimpleInteger, "reset", semian_simple_integer_reset, 0);
   rb_define_method(cSimpleInteger, "value", semian_simple_integer_value_get, 0);
   rb_define_method(cSimpleInteger, "value=", semian_simple_integer_value_set, 1);
@@ -52,39 +48,53 @@ VALUE semian_simple_integer_alloc(VALUE klass)
 }
 
 
-VALUE semian_simple_integer_initialize(VALUE self)
+VALUE semian_simple_integer_initialize(VALUE self, VALUE name)
 {
-  int* data = get_value(self);
-  *data = 0;
+  semian_simple_integer_t *res;
+  TypedData_Get_Struct(self, semian_simple_integer_t, &semian_simple_integer_type, res);
+  res->key = generate_key(to_s(name));
+
+  semian_simple_integer_shared_t* data = get_value(self);
+  data->val = 0;
 
   return self;
 }
 
-VALUE semian_simple_integer_increment(VALUE self, VALUE val) {
-  int *data = get_value(self);
-  *data += rb_num2int(val);
+VALUE semian_simple_integer_increment(int argc, VALUE *argv, VALUE self) {
+  // This is definitely the worst API ever.
+  // https://silverhammermba.github.io/emberb/c/#parsing-arguments
+  VALUE val;
+  rb_scan_args(argc, argv, "01", &val);
+
+  semian_simple_integer_shared_t *data = get_value(self);
+
+  if (NIL_P(val)) {
+    data->val += 1;
+  } else {
+    data->val += RB_NUM2INT(val);
+  }
 
-  return RB_INT2NUM(*data);
+  return RB_INT2NUM(data->val);
 }
 
 VALUE semian_simple_integer_reset(VALUE self) {
-  int *data = get_value(self);
-  *data = 0;
+  semian_simple_integer_shared_t *data = get_value(self);
+  data->val = 0;
 
-  return RB_INT2NUM(*data);
+  return RB_INT2NUM(data->val);
 }
 
 VALUE semian_simple_integer_value_get(VALUE self) {
-  int *data = get_value(self);
-  return RB_INT2NUM(*data);
+  semian_simple_integer_shared_t *data = get_value(self);
+  return RB_INT2NUM(data->val);
 }
 
 VALUE semian_simple_integer_value_set(VALUE self, VALUE val) {
-  int *data = get_value(self);
+  semian_simple_integer_shared_t *data = get_value(self);
 
   // TODO(michaelkipper): Check for respond_to?(:to_i) before calling.
   VALUE to_i = rb_funcall(val, rb_intern("to_i"), 0);
-  *data = RB_NUM2INT(to_i);
+  data->val = RB_NUM2INT(to_i);
 
-  return RB_INT2NUM(*data);
+  return RB_INT2NUM(data->val);
 }

ext/semian/simple_integer.h

@@ -6,8 +6,8 @@
 void Init_SimpleInteger();
 
 VALUE semian_simple_integer_alloc(VALUE klass);
-VALUE semian_simple_integer_initialize(VALUE self);
-VALUE semian_simple_integer_increment(VALUE self, VALUE val);
+VALUE semian_simple_integer_initialize(VALUE self, VALUE name);
+VALUE semian_simple_integer_increment(int argc, VALUE *argv, VALUE self);
 VALUE semian_simple_integer_reset(VALUE self);
 VALUE semian_simple_integer_value_get(VALUE self);
 VALUE semian_simple_integer_value_set(VALUE self, VALUE val);

ext/semian/sysv_semaphores.c

@@ -29,6 +29,7 @@ raise_semian_syscall_error(const char *syscall, int error_num)
 void
 initialize_semaphore_set(semian_resource_t* res, const char* id_str, long permissions, int tickets, double quota)
 {
+
   res->key = generate_key(id_str);
   res->strkey = (char*)  malloc((2 /*for 0x*/+ sizeof(uint64_t) /*actual key*/+ 1 /*null*/) * sizeof(char));
   sprintf(res->strkey, "0x%08x", (unsigned int) res->key);
@@ -51,10 +52,6 @@ initialize_semaphore_set(semian_resource_t* res, const char* id_str, long permis
     }
   }
 
-# if DEBUG
-    printf("[DEBUG] Init semaphore '%s' (key %s) to sem_id %d\n", res->name, res->strkey, res->sem_id);
-# endif
-
   set_semaphore_permissions(res->sem_id, permissions);
 
   /*

ext/semian/types.h

@@ -58,6 +58,16 @@ typedef struct {
   uint64_t key;
 } semian_simple_integer_t;
 
+// Shared simple integer structure
+typedef struct {
+  int val;
+} semian_simple_integer_shared_t;
+
+// Internal simple sliding window structure
+typedef struct {
+  uint64_t key;
+} semian_simple_sliding_window_t;
+
 // Shared simple sliding window structure
 typedef struct {
   int max_size;
@@ -67,9 +77,4 @@ typedef struct {
   int data[SLIDING_WINDOW_MAX_SIZE];
 } semian_simple_sliding_window_shared_t;
 
-// Internal simple sliding window structure
-typedef struct {
-  uint64_t key;
-} semian_simple_sliding_window_t;
-
 #endif // SEMIAN_TYPES_H

lib/semian/circuit_breaker.rb

@@ -23,8 +23,6 @@ def initialize(name, exceptions:, success_threshold:, error_threshold:, error_ti
     end
 
     def acquire(resource = nil, &block)
-      puts "[DEBUG] #{Time.now} - Acquiring resource '#{resource}'"
-
       return yield if disabled?
       transition_to_half_open if transition_to_half_open?
 
@@ -113,7 +111,6 @@ def error_threshold_reached?
     end
 
     def error_timeout_expired?
-      puts "[DEBUG] Checking error_timeout_expired? #{@errors.last}"
       last_error_time = @errors.last
       return false unless last_error_time
       Time.at(last_error_time) + @error_timeout < Time.now
@@ -124,7 +121,6 @@ def push_error(error)
     end
 
     def push_time(window, time: Time.now)
-      puts "[DEBUG] push_time(#{time.to_i}) - rejecting before #{time.to_i - @error_timeout}"
       window.reject! { |err_time| err_time + @error_timeout < time.to_i }
       window << time.to_i
     end

lib/semian/simple_integer.rb

@@ -6,12 +6,11 @@ class Integer #:nodoc:
       attr_accessor :value
 
       def initialize(name)
-        @name = name
-        initialize_simple_integer if respond_to?(:initialize_simple_integer)
+        initialize_simple_integer(name) if respond_to?(:initialize_simple_integer)
         reset
       end
 
-      def increment(val)
+      def increment(val = 1)
         @value += val
       end
 

test/circuit_breaker_test.rb

@@ -11,8 +11,6 @@ def setup
     rescue
       nil
     end
-
-    puts "[DEBUG] Registering semian :testing at #{Time.now} (#{Time.now.to_i})"
     Semian.register(:testing, tickets: 1, exceptions: [SomeError], error_threshold: 2, error_timeout: 5, success_threshold: 1)
     @resource = Semian[:testing]
   end
@@ -42,14 +40,8 @@ def test_after_error_timeout_is_elapsed_requests_are_attempted_again
   end
 
   def test_until_success_threshold_is_reached_a_single_error_will_reopen_the_circuit
-    puts @strio.string
-    puts "[DEBUG] Half open circuit..."
     half_open_cicuit!
-    puts @strio.string
-    puts "[DEBUG] Trigger error..."
     trigger_error!
-    puts @strio.string
-    puts "[DEBUG] Assert circuit opened..."
     assert_circuit_opened
     assert_match(/State transition from open to half_open/, @strio.string)
   end

test/redis_test.rb

@@ -272,9 +272,7 @@ def test_circuit_breaker_on_query
       client.get('foo')
     end
 
-    puts "[DEBUG] #1 Time.now = #{Time.now}"
     Timecop.travel(ERROR_TIMEOUT + 1) do
-      puts "[DEBUG] #2 Time.now = #{Time.now}"
       assert_equal '2', client.get('foo')
     end
   end

test/resource_test.rb

@@ -1,5 +1,4 @@
 require 'test_helper'
-
 require 'objspace'
 
 class TestResource < Minitest::Test
@@ -490,7 +489,7 @@ def test_multiple_register_with_fork
 
   def test_memsize
     r = create_resource :testing, tickets: 1
-    puts "mkipper: Resource size is #{ObjectSpace.memsize_of(r)} byte(s)"
+    assert_equal 128, ObjectSpace.memsize_of(r)
   end
 
   def create_resource(*args)

test/simple_integer_test.rb

@@ -26,7 +26,7 @@ def test_access_value
     def test_increment
       @integer.increment(4)
       assert_equal(4, @integer.value)
-      @integer.increment(1)
+      @integer.increment
       assert_equal(5, @integer.value)
       @integer.increment(-2)
       assert_equal(3, @integer.value)