Cleaned up some module cache misbehaviors. Fixes #535

3 files changed, 73 insertions, 55 deletions

diff --git a/src/ModuleCache.cc b/src/ModuleCache.cc
index 1b7a9e5..595a463 100644
--- a/src/ModuleCache.cc
+++ b/src/ModuleCache.cc
@@ -25,48 +25,61 @@ ModuleCache *ModuleCache::inst = NULL;
 	Reevaluate the given file and recompile if necessary.
 	Returns NULL on any error (e.g. compile error or file not found)
 
-	If the given filename is relative, it means that the module hasn't been
-	previously located.
+	The given filename must be absolute.
 */
 FileModule *ModuleCache::evaluate(const std::string &filename)
 {
-	FileModule *lib_mod = (this->entries.find(filename) != this->entries.end()) ?
-		&(*this->entries[filename].module) : NULL;
-
+	FileModule *lib_mod = NULL;
+	bool found = false;
+	if (this->entries.find(filename) != this->entries.end()) {
+		found = true;
+		lib_mod = this->entries[filename].module;
+	}
+  
 	// Don't try to recursively evaluate - if the file changes
 	// during evaluation, that would be really bad.
 	if (lib_mod && lib_mod->isHandlingDependencies()) return lib_mod;
 
-	bool shouldCompile = true;
-
 	// Create cache ID
 	struct stat st;
 	memset(&st, 0, sizeof(struct stat));
 	bool valid = (stat(filename.c_str(), &st) == 0);
 
-  // If file isn't there, just return and let the cache retain the old module
+	// If file isn't there, just return and let the cache retain the old module
 	if (!valid) return NULL;
-	
+
+	// If the file is present, we'll always cache some result
 	std::string cache_id = str(boost::format("%x.%x") % st.st_mtime % st.st_size);
 
-	// Lookup in cache
-	if (lib_mod) {
-		if (this->entries[filename].cache_id == cache_id) {
+	cache_entry &entry = this->entries[filename];
+	// Initialize entry, if new
+	if (!found) {
+		entry.module = NULL;
+		entry.cache_id = cache_id;
+	}
+  
+	bool shouldCompile = true;
+	if (found) {
+		// Files should only be recompiled if the cache ID changed
+		if (entry.cache_id == cache_id) {
 			shouldCompile = false;
-			if (lib_mod->includesChanged()) {
+			// Recompile if includes changed
+			if (lib_mod && lib_mod->includesChanged()) {
 				lib_mod = NULL;
 				shouldCompile = true;
 			}
 		}
 	}
-	else {
-		shouldCompile = valid;
-	}
+
+#ifdef DEBUG
+	// Causes too much debug output
+	//if (!shouldCompile) PRINTB("Using cached library: %s (%p)", filename % lib_mod);
+#endif
 
 	// If cache lookup failed (non-existing or old timestamp), compile module
 	if (shouldCompile) {
 #ifdef DEBUG
-		if (this->entries.find(filename) != this->entries.end()) {
+		if (found) {
 			PRINTB("Recompiling cached library: %s (%s)", filename % cache_id);
 		}
 		else {
@@ -84,35 +97,25 @@ FileModule *ModuleCache::evaluate(const std::string &filename)
 			textbuf << ifs.rdbuf();
 		}
 		textbuf << "\n" << commandline_commands;
-
+		
 		print_messages_push();
-
-		FileModule *oldmodule = NULL;
-		cache_entry e = { NULL, cache_id };
-		if (this->entries.find(filename) != this->entries.end()) {
-			oldmodule = this->entries[filename].module;
-		}
-		this->entries[filename] = e;
+		
+		FileModule *oldmodule = lib_mod;
 		
 		std::string pathname = boosty::stringy(fs::path(filename).parent_path());
 		lib_mod = dynamic_cast<FileModule*>(parse(textbuf.str().c_str(), pathname.c_str(), false));
 		PRINTB_NOCACHE("  compiled module: %p", lib_mod);
 		
-		if (lib_mod) {
-			// We defer deletion so we can ensure that the new module won't
-			// have the same address as the old
-			delete oldmodule;
-			this->entries[filename].module = lib_mod;
-		} else {
-			this->entries.erase(filename);
-		}
+		// We defer deletion so we can ensure that the new module won't
+		// have the same address as the old
+		if (oldmodule) delete oldmodule;
+		entry.module = lib_mod;
+		entry.cache_id = cache_id;
 		
 		print_messages_pop();
 	}
-
-	if (lib_mod) {
-		lib_mod->handleDependencies();
-	}
+	
+	if (lib_mod) lib_mod->handleDependencies();
 
 	return lib_mod;
 }
@@ -124,7 +127,11 @@ void ModuleCache::clear()
 
 FileModule *ModuleCache::lookup(const std::string &filename)
 {
-	return (this->entries.find(filename) != this->entries.end()) ?
-		&(*this->entries[filename].module) : NULL;
+	return isCached(filename) ? this->entries[filename].module : NULL;
+}
+
+bool ModuleCache::isCached(const std::string &filename)
+{
+	return this->entries.find(filename) != this->entries.end();
 }
 
diff --git a/src/ModuleCache.h b/src/ModuleCache.h
index 7795ab7..e8a4580 100644
--- a/src/ModuleCache.h
+++ b/src/ModuleCache.h
@@ -10,6 +10,7 @@ public:
 	static ModuleCache *instance() { if (!inst) inst = new ModuleCache; return inst; }
 	class FileModule *evaluate(const std::string &filename);
 	class FileModule *lookup(const std::string &filename);
+	bool isCached(const std::string &filename);
 	size_t size() { return this->entries.size(); }
 	void clear();
 
diff --git a/src/module.cc b/src/module.cc
index 1f4059e..dc89a6f 100644
--- a/src/module.cc
+++ b/src/module.cc
@@ -264,42 +264,52 @@ bool FileModule::handleDependencies()
 	this->is_handling_dependencies = true;
 
 	bool changed = false;
+	std::vector<std::string> updates;
 
 	// If a lib in usedlibs was previously missing, we need to relocate it
 	// by searching the applicable paths. We can identify a previously missing module
 	// as it will have a relative path.
-
-	// Iterating manually since we want to modify the container while iterating
-	FileModule::ModuleContainer::iterator iter = this->usedlibs.begin();
-	while (iter != this->usedlibs.end()) {
-		FileModule::ModuleContainer::iterator curr = iter++;
+	BOOST_FOREACH(std::string filename, this->usedlibs) {
 
 		bool wasmissing = false;
+		bool found = true;
+
 		// Get an absolute filename for the module
-		std::string filename = *curr;
 		if (!boosty::is_absolute(filename)) {
 			wasmissing = true;
 			fs::path fullpath = find_valid_path(this->path, filename);
-			if (!fullpath.empty()) filename = boosty::stringy(fullpath);
+			if (!fullpath.empty()) {
+				filename = boosty::stringy(fullpath);
+				updates.push_back(filename);
+				this->usedlibs.erase(filename);
+			}
+			else {
+				found = false;
+			}
 		}
 
-		FileModule *oldmodule = ModuleCache::instance()->lookup(filename);
-		FileModule *newmodule = ModuleCache::instance()->evaluate(filename);
-		// Detect appearance but not removal of files
-		if (newmodule && oldmodule != newmodule) {
-			changed = true;
+		if (found) {
+			bool wascached = ModuleCache::instance()->isCached(filename);
+			FileModule *oldmodule = ModuleCache::instance()->lookup(filename);
+			FileModule *newmodule = ModuleCache::instance()->evaluate(filename);
+			// Detect appearance but not removal of files, and keep old module
+			// on compile errors (FIXME: Is this correct behavior?)
+			if (newmodule && oldmodule != newmodule) {
+				changed = true;
 #ifdef DEBUG
-			PRINTB_NOCACHE("  %s: %p -> %p", filename % oldmodule % newmodule);
+				PRINTB_NOCACHE("  %s: %p -> %p", filename % oldmodule % newmodule);
 #endif
-		}
-		if (!newmodule) {
+			}
 			// Only print warning if we're not part of an automatic reload
-			if (!oldmodule && !wasmissing) {
+			if (!newmodule && !wascached && !wasmissing) {
 				PRINTB_NOCACHE("WARNING: Failed to compile library '%s'.", filename);
 			}
 		}
 	}
 
+	// Relative filenames which were located is reinserted as absolute filenames
+	BOOST_FOREACH(const std::string &filename, updates) this->usedlibs.insert(filename);
+
 	this->is_handling_dependencies = false;
 	return changed;
 }