Merge pull request #554 from ivoanjo/allow-jruby-semaphore-start-at-zero

Fix JRuby Semaphore not allowing #new and #reduce_permits with zero

Author: jdantonio

ext/com/concurrent_ruby/ext/JavaSemaphoreLibrary.java

@@ -40,13 +40,13 @@ public JavaSemaphore(Ruby runtime, RubyClass metaClass) {
 
         @JRubyMethod
         public IRubyObject initialize(ThreadContext context, IRubyObject value) {
-            this.semaphore = new JRubySemaphore(rubyFixnumToInt(value, "count"));
+            this.semaphore = new JRubySemaphore(rubyFixnumToNonNegativeInt(value, "count"));
             return context.nil;
         }
 
         @JRubyMethod
         public IRubyObject acquire(ThreadContext context, IRubyObject value) throws InterruptedException {
-            this.semaphore.acquire(rubyFixnumToInt(value, "permits"));
+            this.semaphore.acquire(rubyFixnumToPositiveInt(value, "permits"));
             return context.nil;
         }
 
@@ -73,14 +73,14 @@ public IRubyObject tryAcquire(ThreadContext context) throws InterruptedException
 
         @JRubyMethod(name = "try_acquire")
         public IRubyObject tryAcquire(ThreadContext context, IRubyObject permits) throws InterruptedException {
-           return getRuntime().newBoolean(semaphore.tryAcquire(rubyFixnumToInt(permits, "permits")));
+           return getRuntime().newBoolean(semaphore.tryAcquire(rubyFixnumToPositiveInt(permits, "permits")));
         }
 
         @JRubyMethod(name = "try_acquire")
         public IRubyObject tryAcquire(ThreadContext context, IRubyObject permits, IRubyObject timeout) throws InterruptedException {
              return getRuntime().newBoolean(
                         semaphore.tryAcquire(
-                                rubyFixnumToInt(permits, "permits"),
+                                rubyFixnumToPositiveInt(permits, "permits"),
                                 rubyNumericToLong(timeout, "timeout"),
                                 java.util.concurrent.TimeUnit.SECONDS)
                 );
@@ -94,22 +94,31 @@ public IRubyObject release(ThreadContext context) {
 
         @JRubyMethod
         public IRubyObject release(ThreadContext context, IRubyObject value) {
-            this.semaphore.release(rubyFixnumToInt(value, "permits"));
+            this.semaphore.release(rubyFixnumToPositiveInt(value, "permits"));
             return getRuntime().newBoolean(true);
         }
 
         @JRubyMethod(name = "reduce_permits")
         public IRubyObject reducePermits(ThreadContext context, IRubyObject reduction) throws InterruptedException {
-            this.semaphore.publicReducePermits(rubyFixnumToInt(reduction, "reduction"));
+            this.semaphore.publicReducePermits(rubyFixnumToNonNegativeInt(reduction, "reduction"));
             return context.nil;
         }
 
-        private int rubyFixnumToInt(IRubyObject value, String paramName) {
+        private int rubyFixnumToNonNegativeInt(IRubyObject value, String paramName) {
+            if (value instanceof RubyFixnum && ((RubyFixnum) value).getLongValue() >= 0) {
+                RubyFixnum fixNum = (RubyFixnum) value;
+                return (int) fixNum.getLongValue();
+            } else {
+                throw getRuntime().newArgumentError(paramName + " must be a non-negative integer");
+            }
+        }
+
+        private int rubyFixnumToPositiveInt(IRubyObject value, String paramName) {
             if (value instanceof RubyFixnum && ((RubyFixnum) value).getLongValue() > 0) {
                 RubyFixnum fixNum = (RubyFixnum) value;
                 return (int) fixNum.getLongValue();
             } else {
-                throw getRuntime().newArgumentError(paramName + " must be in integer greater than zero");
+                throw getRuntime().newArgumentError(paramName + " must be an integer greater than zero");
             }
         }
 
@@ -118,7 +127,7 @@ private long rubyNumericToLong(IRubyObject value, String paramName) {
                 RubyNumeric fixNum = (RubyNumeric) value;
                 return fixNum.getLongValue();
             } else {
-                throw getRuntime().newArgumentError(paramName + " must be in float greater than zero");
+                throw getRuntime().newArgumentError(paramName + " must be a float greater than zero");
             }
         }
 

spec/concurrent/atomic/semaphore_spec.rb

@@ -1,12 +1,20 @@
 shared_examples :semaphore do
   let(:semaphore) { described_class.new(3) }
 
-  context '#initialize' do
+  describe '#initialize' do
     it 'raises an exception if the initial count is not an integer' do
       expect {
         described_class.new('foo')
       }.to raise_error(ArgumentError)
     end
+
+    context 'when initializing with 0' do
+      let(:semaphore) { described_class.new(0) }
+
+      it do
+        expect(semaphore).to_not be nil
+      end
+    end
   end
 
   describe '#acquire' do
@@ -27,6 +35,14 @@
         expect(semaphore.available_permits).to eq 0
       end
     end
+
+    context 'when acquiring zero permits' do
+      it do
+        expect {
+          semaphore.acquire(0)
+        }.to raise_error(ArgumentError)
+      end
+    end
   end
 
   describe '#drain_permits' do
@@ -54,6 +70,14 @@
         result = semaphore.try_acquire(20)
         expect(result).to be_falsey
       end
+
+      context 'when trying to acquire zero permits' do
+        it do
+          expect {
+            semaphore.try_acquire(0)
+          }.to raise_error(ArgumentError)
+        end
+      end
     end
 
     context 'with timeout' do
@@ -86,7 +110,7 @@
 
     it 'reduces permits below zero' do
       semaphore.reduce_permits 1003
-      expect(semaphore.available_permits).to eq -1000
+      expect(semaphore.available_permits).to eq(-1000)
     end
 
     it 'reduces permits' do
@@ -95,6 +119,33 @@
       semaphore.reduce_permits 2
       expect(semaphore.available_permits).to eq 0
     end
+
+    it 'reduces zero permits' do
+      semaphore.reduce_permits 0
+      expect(semaphore.available_permits).to eq 3
+    end
+  end
+
+  describe '#release' do
+    it 'increases the number of available permits by one' do
+      semaphore.release
+      expect(semaphore.available_permits).to eq 4
+    end
+
+    context 'when a number of permits is specified' do
+      it 'increases the number of available permits by the specified value' do
+        semaphore.release(2)
+        expect(semaphore.available_permits).to eq 5
+      end
+
+      context 'when permits is set to zero' do
+        it do
+          expect {
+            semaphore.release(0)
+          }.to raise_error(ArgumentError)
+        end
+      end
+    end
   end
 end