Review fixes

Author: Michael Kipper

ext/semian/simple_integer.c

@@ -97,7 +97,7 @@ semian_simple_integer_increment(int argc, VALUE *argv, VALUE self)
   rb_scan_args(argc, argv, "01", &val);
 
   int value = check_increment_arg(val);
-  if (perform_semop(res->sem_id, 0, value, SEM_UNDO, NULL) == -1) {
+  if (perform_semop(res->sem_id, 0, value, 0, NULL) == -1) {
     rb_raise(eInternal, "error incrementing simple integer, errno: %d (%s)", errno, strerror(errno));
   }
 

ext/semian/sliding_window.c

@@ -28,14 +28,6 @@ static const rb_data_type_t semian_simple_sliding_window_type = {
   .flags = RUBY_TYPED_FREE_IMMEDIATELY,
 };
 
-static void init_fn(void* ptr)
-{
-  semian_simple_sliding_window_shared_t* res = (semian_simple_sliding_window_shared_t*)ptr;
-  res->max_size = 0;
-  res->length = 0;
-  res->start = 0;
-}
-
 static int
 check_max_size_arg(VALUE max_size)
 {
@@ -93,7 +85,10 @@ check_scale_factor_arg(VALUE scale_factor)
 static VALUE
 grow_window(int sem_id, semian_simple_sliding_window_shared_t* window, int new_max_size)
 {
-  if (new_max_size > SLIDING_WINDOW_MAX_SIZE) return Qnil;
+  if (new_max_size > SLIDING_WINDOW_MAX_SIZE) {
+    sem_meta_unlock(sem_id);
+    rb_raise(rb_eArgError, "Cannot grow window to %d (MAX_SIZE=%d)", new_max_size, SLIDING_WINDOW_MAX_SIZE);
+  }
 
   int end = window->max_size ? (window->start + window->length) % window->max_size : 0;
   dprintf("Growing window - sem_id:%d start:%d end:%d length:%d max_size:%d new_max_size:%d", sem_id, window->start, end, window->length, window->max_size, new_max_size);
@@ -127,7 +122,10 @@ static void swap(int *a, int *b) {
 static VALUE
 shrink_window(int sem_id, semian_simple_sliding_window_shared_t* window, int new_max_size)
 {
-  if (new_max_size > SLIDING_WINDOW_MAX_SIZE) return Qnil;
+  if (new_max_size > SLIDING_WINDOW_MAX_SIZE) {
+    sem_meta_unlock(sem_id);
+    rb_raise(rb_eArgError, "Cannot shrink window to %d (MAX_SIZE=%d)", new_max_size, SLIDING_WINDOW_MAX_SIZE);
+  }
 
   int new_length = (new_max_size > window->length) ? window->length : new_max_size;
 
@@ -256,7 +254,7 @@ semian_simple_sliding_window_initialize(VALUE self, VALUE name, VALUE max_size,
 
   dprintf("Initializing simple sliding window '%s' (key: %lu)", buffer, res->key);
   res->sem_id = initialize_single_semaphore(res->key, SEM_DEFAULT_PERMISSIONS, 1);
-  res->shmem = get_or_create_shared_memory(res->key, init_fn);
+  res->shmem = get_or_create_shared_memory(res->key);
   res->error_threshold = check_max_size_arg(max_size);
   res->scale_factor = check_scale_factor_arg(scale_factor);
 
@@ -396,8 +394,8 @@ semian_simple_sliding_window_clear(VALUE self)
   return self;
 }
 
+#if 0
 // Handy for debugging the sliding window, but too noisy for regular debugging.
-/*
 static void dprint_window(semian_simple_sliding_window_shared_t *window)
 {
   dprintf("---");
@@ -407,7 +405,7 @@ static void dprint_window(semian_simple_sliding_window_shared_t *window)
   }
   dprintf("---");
 }
-*/
+#endif
 
 VALUE
 semian_simple_sliding_window_reject(VALUE self)

ext/semian/sysv_semaphores.c

@@ -214,7 +214,7 @@ wait_for_new_semaphore_set(uint64_t key, long permissions)
   union semun sem_opts;
   sem_opts.buf = &sem_ds;
 
-  int sem_id = semget(key, 1, permissions);
+  int sem_id = semget((key_t)key, 1, permissions);
   if (sem_id == -1){
       raise_semian_syscall_error("semget()", errno);
   }

ext/semian/sysv_shared_memory.c

@@ -2,45 +2,33 @@
 
 #include "util.h"
 
-#define TIMEOUT_MS (5 * 1e6)
-#define WAIT_MS (10)
-#define RETRIES (TIMEOUT_MS / WAIT_MS)
-
 void*
-wait_for_shared_memory(uint64_t key)
-{
-  for (int i = 0; i < RETRIES; ++i) {
-    int shmid = shmget(key, SHM_DEFAULT_SIZE, SHM_DEFAULT_PERMISSIONS);
-    if (shmid != -1) {
-      return shmat(shmid, NULL, 0);
-    }
-    usleep(WAIT_MS);
-  }
-
-  rb_raise(rb_eArgError, "could not get shared memory");
-}
-
-void*
-get_or_create_shared_memory(uint64_t key, shared_memory_init_fn fn)
+get_or_create_shared_memory(uint64_t key)
 {
   void* shmem = NULL;
   if (!key) return NULL;
 
   dprintf("Creating shared memory (key: %lu)", key);
   int shmid = shmget(key, SHM_DEFAULT_SIZE, IPC_CREAT | IPC_EXCL | SHM_DEFAULT_PERMISSIONS);
   if (shmid != -1) {
-    dprintf("Created shared memory (key: %lu)", key);
-
+    dprintf("Created shared memory (key:%lu sem_id:%d)", key, shmid);
     shmem = shmat(shmid, NULL, 0);
     if (shmem == (void*)-1) {
       rb_raise(rb_eArgError, "could not get shared memory (%s)", strerror(errno));
     }
 
-    if (fn) fn(shmem);
-
     shmctl(key, IPC_RMID, NULL);
   } else {
-    shmem = wait_for_shared_memory(key);
+    shmid = shmget(key, SHM_DEFAULT_SIZE, SHM_DEFAULT_PERMISSIONS);
+    if (shmid == -1) {
+      rb_raise(rb_eArgError, "could not get shared memory (%s)", strerror(errno));
+    }
+
+    dprintf("Got shared memory (key:%lu sem_id:%d)", key, shmid);
+    shmem = shmat(shmid, NULL, 0);
+    if (shmem == (void*)-1) {
+      rb_raise(rb_eArgError, "could not get shared memory (%s)", strerror(errno));
+    }
   }
 
   return shmem;

ext/semian/sysv_shared_memory.h

@@ -8,12 +8,10 @@
 
 // Default permissions for shared memory
 #define SHM_DEFAULT_PERMISSIONS 0660
-#define SHM_DEFAULT_SIZE 1024
-
-typedef void (*shared_memory_init_fn)(void*);
+#define SHM_DEFAULT_SIZE 4096
 
 void*
-get_or_create_shared_memory(uint64_t key, shared_memory_init_fn fn);
+get_or_create_shared_memory(uint64_t key);
 
 void
 free_shared_memory(void* key);

ext/semian/test/Makefile

@@ -0,0 +1,18 @@
+# Makefile
+
+CC := gcc -std=gnu99 -I. -I..
+TESTS := types_test
+
+all : test
+
+test : types_test
+
+types_test : types_test.c
+	$(CC) -o $@ $<
+	./$@
+
+clean :
+	rm -f *.o
+	rm -f $(TESTS)
+
+.PHONY : all test types_test clean

ext/semian/test/types_test.c

@@ -0,0 +1,36 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "types.h"
+
+void assert_equal_int(int actual, int expected, const char *message)
+{
+  if (actual != expected) {
+    fprintf(stderr, "Error: got %d, expected %d (%s)\n", actual, expected, message);
+    exit(EXIT_FAILURE);
+  }
+}
+
+void assert_le_int(int actual, int expected, const char *message)
+{
+  if (actual > expected) {
+    fprintf(stderr, "Error: got %d, which is greater than %d (%s)\n", actual, expected, message);
+    exit(EXIT_FAILURE);
+  }
+}
+
+void test_sliding_window()
+{
+  semian_simple_sliding_window_shared_t window;
+  assert_le_int(sizeof(window), 4096, "window size is greater than a page");
+  assert_equal_int(sizeof(window.data), SLIDING_WINDOW_MAX_SIZE * sizeof(int), "window data size");
+}
+
+int main(int argc, char **argv)
+{
+  printf("Info: Running test\n");
+
+  test_sliding_window();
+
+  return EXIT_SUCCESS;
+}

ext/semian/types.h

@@ -10,7 +10,7 @@ For custom type definitions specific to semian
 #include <sys/sem.h>
 #include <sys/time.h>
 
-#define SLIDING_WINDOW_MAX_SIZE 4096
+#define SLIDING_WINDOW_MAX_SIZE 1000
 
 // For sysV semop syscals
 // see man semop

test/simple_sliding_window_test.rb

@@ -218,9 +218,18 @@ def test_scale_factor
     end
   end
 
-  def test_max_size
-    8192.times do |i|
-      @sliding_window << i
+  def test_huge_sliding_window
+    id = Time.now.strftime('%H:%M:%S.%N')
+    window = ::Semian::ThreadSafe::SlidingWindow.new(id, max_size: 1000)
+    4000.times do |i|
+      window << i
+    end
+  end
+
+  def test_huge_sliding_window_fails
+    id = Time.now.strftime('%H:%M:%S.%N')
+    assert_raises ArgumentError do
+      ::Semian::ThreadSafe::SlidingWindow.new(id, max_size: 1001)
     end
   end