Merge pull request #683 from MartinNowak/fix674

fix #674 - cache needs to be locked for concurrent dub runs

source/dub/dub.d

@@ -575,20 +575,30 @@ class Dub {
 		if (m_dryRun) return null;
 
 		logDiagnostic("Acquiring package zip file");
-		auto dload = m_projectPath ~ ".dub/temp/downloads";
-		auto tempfname = packageId ~ "-" ~ (ver.startsWith('~') ? ver[1 .. $] : ver) ~ ".zip";
-		auto tempFile = m_tempPath ~ tempfname;
-		string sTempFile = tempFile.toNativeString();
-		if (exists(sTempFile)) std.file.remove(sTempFile);
-		supplier.retrievePackage(tempFile, packageId, dep, (options & FetchOptions.usePrerelease) != 0); // Q: continue on fail?
-		scope(exit) std.file.remove(sTempFile);
-
-		logInfo("Placing %s %s to %s...", packageId, ver, placement.toNativeString());
+
 		auto clean_package_version = ver[ver.startsWith("~") ? 1 : 0 .. $];
 		clean_package_version = clean_package_version.replace("+", "_"); // + has special meaning for Optlink
+		if (!placement.existsFile())
+			mkdirRecurse(placement.toNativeString());
 		Path dstpath = placement ~ (packageId ~ "-" ~ clean_package_version);
 
-		return m_packageManager.storeFetchedPackage(tempFile, pinfo, dstpath);
+		if (auto lock = tryLockFile(dstpath.toNativeString() ~ ".lock")) // avoid concurrent fetch
+		{
+			auto path = getTempFile(packageId, ".zip");
+			supplier.retrievePackage(path, packageId, dep, (options & FetchOptions.usePrerelease) != 0); // Q: continue on fail?
+			scope(exit) std.file.remove(path.toNativeString());
+
+			logInfo("Placing %s %s to %s...", packageId, ver, placement.toNativeString());
+
+			return m_packageManager.storeFetchedPackage(path, pinfo, dstpath);
+		}
+		else
+		{
+			logInfo("Waiting for concurrent dub to fetch %s %s.", packageId, ver);
+			lockFile(dstpath.toNativeString() ~ ".lock", 30.seconds); // wait for other dub instance
+			m_packageManager.refresh(false);
+			return m_packageManager.getPackage(packageId, ver, dstpath);
+		}
 	}
 
 	/// Removes a given package from the list of present/cached modules.

source/dub/internal/utils.d

@@ -14,6 +14,7 @@ import dub.internal.vibecompat.inet.url;
 import dub.version_;
 
 // todo: cleanup imports.
+import core.thread;
 import std.algorithm : startsWith;
 import std.array;
 import std.conv;
@@ -27,13 +28,13 @@ import std.zip;
 version(DubUseCurl) import std.net.curl;
 
 
+private Path[] temporary_files;
+
 Path getTempDir()
 {
 	return Path(std.file.tempDir());
 }
 
-private Path[] temporary_files;
-
 Path getTempFile(string prefix, string extension = null)
 {
 	import std.uuid : randomUUID;
@@ -43,11 +44,45 @@ Path getTempFile(string prefix, string extension = null)
 	return path;
 }
 
+// lockfile based on atomic mkdir
+struct LockFile
+{
+	bool opCast(T:bool)() { return !!path; }
+	~this() { if (path) rmdir(path); }
+	string path;
+}
+
+auto tryLockFile(string path)
+{
+	import std.file;
+	if (collectException(mkdir(path)))
+		return LockFile(null);
+	return LockFile(path);
+}
+
+auto lockFile(string path, Duration wait)
+{
+	import std.datetime, std.file;
+	auto t0 = Clock.currTime();
+	auto dur = 1.msecs;
+	while (true)
+	{
+		if (!collectException(mkdir(path)))
+			return LockFile(path);
+		enforce(Clock.currTime() - t0 < wait, "Failed to lock '"~path~"'.");
+		if (dur < 1024.msecs) // exponentially increase sleep time
+			dur *= 2;
+		Thread.sleep(dur);
+	}
+}
+
 static ~this()
 {
 	foreach (path; temporary_files)
 	{
-		std.file.remove(path.toNativeString());
+		auto spath = path.toNativeString();
+		if (spath.exists)
+			std.file.remove(spath);
 	}
 }
 
@@ -127,18 +162,18 @@ void runCommands(in string[] commands, string[string] env = null)
 	version(Windows) enum nullFile = "NUL";
 	else version(Posix) enum nullFile = "/dev/null";
 	else static assert(0);
-	
+
 	auto childStdout = stdout;
 	auto childStderr = stderr;
 	auto config = Config.retainStdout | Config.retainStderr;
-	
+
 	// Disable child's stdout/stderr depending on LogLevel
 	auto logLevel = getLogLevel();
 	if(logLevel >= LogLevel.warn)
 		childStdout = File(nullFile, "w");
 	if(logLevel >= LogLevel.none)
 		childStderr = File(nullFile, "w");
-	
+
 	foreach(cmd; commands){
 		logDiagnostic("Running %s", cmd);
 		Pid pid;
@@ -290,7 +325,7 @@ auto fuzzySearch(R)(R strings, string input){
 /**
 	If T is a bitfield-style enum, this function returns a string range
 	listing the names of all members included in the given value.
-	
+
 	Example:
 	---------
 	enum Bits {
@@ -300,7 +335,7 @@ auto fuzzySearch(R)(R strings, string input){
 		c = 1<<2,
 		a_c = a | c,
 	}
-	
+
 	assert( bitFieldNames(Bits.none).equals(["none"]) );
 	assert( bitFieldNames(Bits.a).equals(["a"]) );
 	assert( bitFieldNames(Bits.a_c).equals(["a", "c", "a_c"]) );

source/dub/packagesupplier.d

@@ -170,7 +170,7 @@ class RegistryPackageSupplier : PackageSupplier {
 			if (!cacheDir.existsFile())
 				mkdirRecurse(cacheDir.toNativeString());
 			// TODO: method is slow due to Json escaping
-			writeJsonFile(path, m_metadataCache.serializeToJson());
+			atomicWriteJsonFile(path, m_metadataCache.serializeToJson());
 			break;
 
 		case CacheOp.load:

test/issue674-concurrent-dub.sh

@@ -0,0 +1,21 @@
+#!/bin/bash
+
+set -e -o pipefail
+
+TMPDIR=$(mktemp -d)
+echo ${TMPDIR}
+
+function cleanup {
+    rm -rf ${TMPDIR}
+}
+trap cleanup EXIT
+
+cd ${TMPDIR} && $DUB fetch --cache=local bloom &
+pid1=$!
+cd ${TMPDIR} && $DUB fetch --cache=local bloom &
+pid2=$!
+wait $pid1
+wait $pid2
+if [ ! -d ${TMPDIR}/bloom* ]; then
+    exit 1
+fi