Browse Source

[java] Fixed some potential race conditions in java.vm.Lock. Closes #3767

Cauê Waneck 10 years ago
parent
commit
0b8914e7c8
4 changed files with 86 additions and 43 deletions
  1. 2 1
      std/java/_std/Sys.hx
  2. 34 21
      std/java/vm/Lock.hx
  3. 0 21
      tests/unit/src/unit/TestJava.hx
  4. 50 0
      tests/unit/src/unit/issues/Issue3767.hx

+ 2 - 1
std/java/_std/Sys.hx

@@ -22,6 +22,7 @@
 
 import java.lang.System;
 import sys.io.Process;
+using haxe.Int64;
 
 @:coreApi class Sys {
 	private static var _args:java.NativeArray<String>;
@@ -125,7 +126,7 @@ import sys.io.Process;
 
 	public static function time() : Float
 	{
-		return cast(System.currentTimeMillis(), Float) / 1000;
+		return cast(System.currentTimeMillis().div(Int64.ofInt(1000)), Float);
 	}
 
 	public static function cpuTime() : Float

+ 34 - 21
std/java/vm/Lock.hx

@@ -1,5 +1,7 @@
 package java.vm;
 import java.Lib;
+import java.lang.System;
+using haxe.Int64;
 
 @:native('haxe.java.vm.Lock') class Lock
 {
@@ -19,37 +21,48 @@ import java.Lib;
 	public function wait(?timeout : Float) : Bool
 	{
 		var ret = false;
-		untyped __lock__(this,
+		java.Lib.lock(this,
 		{
-			if (--releasedCount >= 0)
-				return true;
-			if (timeout == null)
+			if (--releasedCount < 0)
 			{
-				while( releasedCount < 0 )
+				if (timeout == null)
 				{
-					try
+					// since .notify() is asynchronous, this `while` is needed
+					// because there is a very remote possibility of release() awaking a thread,
+					// but before it releases, another thread calls wait - and since the release count
+					// is still positive, it will get the lock.
+					while( releasedCount < 0 )
 					{
-						untyped __java__("this.wait()");
+						try
+						{
+							untyped __java__("this.wait()");
+						}
+						catch(e:java.lang.InterruptedException)
+						{
+						}
 					}
-					catch(e:java.lang.InterruptedException)
+				} else {
+					var timeout:haxe.Int64 = cast timeout * 1000;
+					var cur = System.currentTimeMillis(),
+					    max = cur.add(timeout);
+					// see above comment about this while loop
+					while ( releasedCount < 0 && cur.compare(max) < 0 )
 					{
+						try
+						{
+							var t = max.sub(cur);
+							untyped __java__("this.wait({0})",t);
+							cur = System.currentTimeMillis();
+						}
+						catch(e:java.lang.InterruptedException)
+						{
+						}
 					}
 				}
-			} else {
-				try
-				{
-					var t:haxe.Int64 = cast timeout * 1000;
-					untyped __java__("this.wait({0})",t);
-				}
-				catch(e:java.lang.InterruptedException)
-				{
-				}
 			}
 			ret = this.releasedCount >= 0;
-			if (++this.releasedCount == 0) //even if timeout failed, we should release the lock; Other locks may have been released in the meantime
-			{
-				untyped this.notify();
-			}
+			if (!ret)
+				this.releasedCount++; //timed out
 		});
 		return ret;
 	}

+ 0 - 21
tests/unit/src/unit/TestJava.hx

@@ -31,27 +31,6 @@ class TestJava extends Test
 		t(haxe.uppercasepackage.Lowercase.lowercaseFound);
 	}
 
-	function testBasicLock()
-	{
-		var lock = new Lock();
-		//it starts locked
-		f(lock.wait(0.001));
-		lock.release();
-		t(lock.wait(.001));
-		f(lock.wait(.001));
-		f(lock.wait(.001));
-
-		lock.release();
-		t(lock.wait());
-		lock.release();
-		lock.release();
-		lock.release();
-		t(lock.wait());
-		t(lock.wait(.001));
-		t(lock.wait());
-		f(lock.wait(.001));
-	}
-
 	function testEnumSet()
 	{
 		var es1:EnumSet<TEnum> = EnumSet.noneOf(java.Lib.toNativeEnum(TEnum));

+ 50 - 0
tests/unit/src/unit/issues/Issue3767.hx

@@ -0,0 +1,50 @@
+package unit.issues;
+import unit.Test;
+#if java
+import java.vm.*;
+#elseif neko
+import neko.vm.*;
+#elseif cpp
+import cpp.vm.*;
+#end
+
+class Issue3767 extends Test
+{
+#if (java || neko || cpp)
+	function testBasicLock()
+	{
+		var lock = new Lock();
+		//it starts locked
+		f(lock.wait(0.001));
+		lock.release();
+		t(lock.wait(.001));
+		f(lock.wait(.001));
+		f(lock.wait(.001));
+
+		lock.release();
+		t(lock.wait());
+		lock.release();
+		lock.release();
+		lock.release();
+		t(lock.wait());
+		t(lock.wait(.001));
+		t(lock.wait());
+		f(lock.wait(.001));
+
+		var cur = Sys.time();
+		Thread.create(function()
+		{
+			Sys.sleep(.01);
+			lock.release();
+		});
+		t(lock.wait(2.0));
+		t( (Sys.time() - cur) < 2 );
+		Thread.create(function()
+		{
+			Sys.sleep(.01);
+			lock.release();
+		});
+		t(lock.wait());
+	}
+#end
+}