Fixed a bug where changing a file during a large automatic reload could cause a crash

6 files changed, 41 insertions, 30 deletions

diff --git a/src/ModuleCache.cc b/src/ModuleCache.cc
index 83f7b7d..99c0b20 100644
--- a/src/ModuleCache.cc
+++ b/src/ModuleCache.cc
@@ -30,8 +30,14 @@ ModuleCache *ModuleCache::inst = NULL;
 */
 FileModule *ModuleCache::evaluate(const std::string &filename)
 {
+	FileModule *lib_mod = (this->entries.find(filename) != this->entries.end()) ?
+		&(*this->entries[filename].module) : NULL;
+
+	// 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;
-	FileModule *lib_mod = NULL;
 
 	// Create cache ID
 	struct stat st;
@@ -44,8 +50,7 @@ FileModule *ModuleCache::evaluate(const std::string &filename)
 	std::string cache_id = str(boost::format("%x.%x") % st.st_mtime % st.st_size);
 
 	// Lookup in cache
-	if (this->entries.find(filename) != this->entries.end()) {
-		lib_mod = &(*this->entries[filename].module);
+	if (lib_mod) {
 		if (this->entries[filename].cache_id == cache_id) {
 			shouldCompile = false;
 
@@ -104,7 +109,9 @@ FileModule *ModuleCache::evaluate(const std::string &filename)
 		print_messages_pop();
 	}
 
-	if (lib_mod) lib_mod->handleDependencies();
+	if (lib_mod) {
+		lib_mod->handleDependencies();
+	}
 
 	return lib_mod;
 }
@@ -114,3 +121,9 @@ void ModuleCache::clear()
 	this->entries.clear();
 }
 
+FileModule *ModuleCache::lookup(const std::string &filename)
+{
+	return (this->entries.find(filename) != this->entries.end()) ?
+		&(*this->entries[filename].module) : NULL;
+}
+
diff --git a/src/ModuleCache.h b/src/ModuleCache.h
index b8ded38..7795ab7 100644
--- a/src/ModuleCache.h
+++ b/src/ModuleCache.h
@@ -9,6 +9,7 @@ class ModuleCache
 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);
 	size_t size() { return this->entries.size(); }
 	void clear();
 
diff --git a/src/modcontext.cc b/src/modcontext.cc
index 3b957fd..8799af9 100644
--- a/src/modcontext.cc
+++ b/src/modcontext.cc
@@ -4,6 +4,7 @@
 #include "function.h"
 #include "printutils.h"
 #include "builtin.h"
+#include "ModuleCache.h"
 
 #include <boost/foreach.hpp>
 
@@ -125,17 +126,18 @@ Value FileContext::evaluate_function(const std::string &name, const EvalContext
 	if (foundf) return foundf->evaluate(this, evalctx);
 	
 	BOOST_FOREACH(const FileModule::ModuleContainer::value_type &m, this->usedlibs) {
-		// m.second is NULL if the library wasn't be compiled (error or file-not-found)
-		if (m.second && 
-				m.second->scope.functions.find(name) != m.second->scope.functions.end()) {
-			FileContext ctx(*m.second, this->parent);
-			ctx.initializeModule(*m.second);
+		// usedmod is NULL if the library wasn't be compiled (error or file-not-found)
+		FileModule *usedmod = ModuleCache::instance()->lookup(m);
+		if (usedmod && 
+				usedmod->scope.functions.find(name) != usedmod->scope.functions.end()) {
+			FileContext ctx(*usedmod, this->parent);
+			ctx.initializeModule(*usedmod);
 			// FIXME: Set document path
 #if 0 && DEBUG
 			PRINTB("New lib Context for %s func:", name);
 			ctx.dump(NULL, NULL);
 #endif
-			return m.second->scope.functions[name]->evaluate(&ctx, evalctx);
+			return usedmod->scope.functions[name]->evaluate(&ctx, evalctx);
 		}
 	}
 
@@ -148,17 +150,18 @@ AbstractNode *FileContext::instantiate_module(const ModuleInstantiation &inst, c
 	if (foundm) return foundm->instantiate(this, &inst, evalctx);
 
 	BOOST_FOREACH(const FileModule::ModuleContainer::value_type &m, this->usedlibs) {
-		// m.second is NULL if the library wasn't be compiled (error or file-not-found)
-		if (m.second && 
-				m.second->scope.modules.find(inst.name()) != m.second->scope.modules.end()) {
-			FileContext ctx(*m.second, this->parent);
-			ctx.initializeModule(*m.second);
+		FileModule *usedmod = ModuleCache::instance()->lookup(m);
+		// usedmod is NULL if the library wasn't be compiled (error or file-not-found)
+		if (usedmod && 
+				usedmod->scope.modules.find(inst.name()) != usedmod->scope.modules.end()) {
+			FileContext ctx(*usedmod, this->parent);
+			ctx.initializeModule(*usedmod);
 			// FIXME: Set document path
 #if 0 && DEBUG
 			PRINT("New file Context:");
 			ctx.dump(NULL, &inst);
 #endif
-			return m.second->scope.modules[inst.name()]->instantiate(&ctx, &inst, evalctx);
+			return usedmod->scope.modules[inst.name()]->instantiate(&ctx, &inst, evalctx);
 		}
 	}
 
diff --git a/src/module.cc b/src/module.cc
index 046d0c4..8fb8506 100644
--- a/src/module.cc
+++ b/src/module.cc
@@ -264,43 +264,35 @@ bool FileModule::handleDependencies()
 	// as it will have a relative path.
 
 	// Iterating manually since we want to modify the container while iterating
-	std::vector<std::pair<std::string, FileModule*> > modified_modules;
 	FileModule::ModuleContainer::iterator iter = this->usedlibs.begin();
 	while (iter != this->usedlibs.end()) {
 		FileModule::ModuleContainer::iterator curr = iter++;
-		FileModule *oldmodule = curr->second;
 
 		bool wasmissing = false;
 		// Get an absolute filename for the module
-		std::string filename = curr->first;
+		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);
 		}
 
+		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;
 #ifdef DEBUG
-			PRINTB_NOCACHE("  %s: %p", filename % newmodule);
+			PRINTB_NOCACHE("  %s: %p -> %p", filename % oldmodule % newmodule);
 #endif
 		}
-		if (newmodule) {
-			modified_modules.push_back(std::make_pair(filename, newmodule));
-			this->usedlibs.erase(curr);
-		}
-		else {
+		if (!newmodule) {
 			// Only print warning if we're not part of an automatic reload
 			if (!oldmodule && !wasmissing) {
 				PRINTB_NOCACHE("WARNING: Failed to compile library '%s'.", filename);
 			}
 		}
 	}
-	BOOST_FOREACH(const FileModule::ModuleContainer::value_type &mod, modified_modules) {
-		this->usedlibs[mod.first] = mod.second;
-	}
 
 	this->is_handling_dependencies = false;
 	return changed;
diff --git a/src/module.h b/src/module.h
index e28677a..6027fe6 100644
--- a/src/module.h
+++ b/src/module.h
@@ -5,6 +5,7 @@
 #include <vector>
 #include <list>
 #include <boost/unordered_map.hpp>
+#include <boost/unordered_set.hpp>
 #include <time.h>
 #include <sys/stat.h>
 
@@ -94,8 +95,9 @@ public:
 	virtual AbstractNode *instantiate(const Context *ctx, const ModuleInstantiation *inst, const EvalContext *evalctx = NULL) const;
 	bool hasIncludes() const { return !this->includes.empty(); }
 	bool usesLibraries() const { return !this->usedlibs.empty(); }
+	bool isHandlingDependencies() const { return this->is_handling_dependencies; }
 
-	typedef boost::unordered_map<std::string, class FileModule*> ModuleContainer;
+	typedef boost::unordered_set<std::string> ModuleContainer;
 	ModuleContainer usedlibs;
 private:
 	struct IncludeFile {
diff --git a/src/parser.y b/src/parser.y
index 2b07f14..0977efa 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -127,7 +127,7 @@
 
 input: 
 /* empty */ |
-TOK_USE { rootmodule->usedlibs[$1] = NULL; } input |
+TOK_USE { rootmodule->usedlibs.insert($1); } input |
 statement input ;
 
 inner_input: