Make ReadWriteLock non-recursive and add *_NoLock() methods to Engine as needed.

Split Module::bypass() into isBypass/setBypass().
Add more documentation to Engine methods.

include/engine/Engine.hpp

@@ -13,9 +13,9 @@ namespace engine {
 
 /** Manages Modules and Cables and steps them in time.
 
-All methods are thread-safe and can safely be called from anywhere.
-However, the following methods obtain a write-lock, so they cannot be called with the engine thread (e.g. inside Module::process()):
-clear, stepBlock, setPrimaryModule, setSampleRate, addModule, removeModule, resetModule, randomizeModule, bypassModule, moduleFromJson, addCable, removeCable, addParamHandle, removeParamHandle, and fromJson
+Engine contains a read/write mutex that locks when the Engine state is being read or written (manipulated).
+Methods that read-lock (stated in their documentation) can be called simultaneously with other read-locking methods.
+Methods that write-lock cannot be called simultaneously or recursively with another read-locking or write-locking method.
 */
 struct Engine {
 	struct Internal;
@@ -24,15 +24,20 @@ struct Engine {
 	Engine();
 	~Engine();
 
-	/** Removes all modules and cables. */
+	/** Removes all modules and cables.
+	Write-locks.
+	*/
 	void clear();
+	PRIVATE void clear_NoLock();
 	/** Advances the engine by `frames` frames.
 	Only call this method from the primary module.
+	Read-locks. Also locks so only one stepBlock() can be called simultaneously or recursively.
 	*/
 	void stepBlock(int frames);
 	/** Module does not need to belong to the Engine.
 	However, Engine will unset the primary module when it is removed from the Engine.
 	NULL will unset the primary module.
+	Write-locks.
 	*/
 	void setPrimaryModule(Module* module);
 	Module* getPrimaryModule();
@@ -40,7 +45,13 @@ struct Engine {
 	/** Returns the sample rate used by the engine for stepping each module.
 	*/
 	float getSampleRate();
+	/** Sets the sample rate to step the modules.
+	Write-locks.
+	*/
 	PRIVATE void setSampleRate(float sampleRate);
+	/** Sets the sample rate if the sample rate in the settings is "Auto".
+	Write-locks.
+	*/
 	void setSuggestedSampleRate(float suggestedSampleRate);
 	/** Returns the inverse of the current sample rate.
 	*/
@@ -78,37 +89,85 @@ struct Engine {
 	/** Fills `moduleIds` with up to `len` module IDs in the rack.
 	Returns the number of IDs written.
 	This C-like method does no allocations. The vector C++ version below does.
+	Read-locks.
 	*/
 	size_t getModuleIds(int64_t* moduleIds, size_t len);
+	/** Returns a vector of module IDs in the rack.
+	Read-locks.
+	*/
 	std::vector<int64_t> getModuleIds();
-	/** Adds a module to the rack engine.
-	The module ID must not be taken by another module.
+	/** Adds a Module to the rack.
+	The module ID must not be taken by another Module.
 	If the module ID is -1, an ID is automatically assigned.
 	Does not transfer pointer ownership.
+	Write-locks.
 	*/
 	void addModule(Module* module);
+	/** Removes a Module from the rack.
+	Write-locks.
+	*/
 	void removeModule(Module* module);
+	PRIVATE void removeModule_NoLock(Module* module);
+	/** Checks whether a Module is in the rack.
+	Read-locks.
+	*/
 	bool hasModule(Module* module);
+	/** Returns the Module with the given ID in the rack.
+	Read-locks.
+	*/
 	Module* getModule(int64_t moduleId);
+	/** Triggers a ResetEvent for the given Module.
+	Write-locks.
+	*/
 	void resetModule(Module* module);
+	/** Triggers a RandomizeEvent for the given Module.
+	Write-locks.
+	*/
 	void randomizeModule(Module* module);
+	/** Sets the bypass state and triggers a BypassEvent or UnBypassEvent of the given Module.
+	Write-locks.
+	*/
 	void bypassModule(Module* module, bool bypass);
-	/** Serializes/deserializes with locking, ensuring that Module::process() is not called during toJson()/fromJson().
+	/** Serializes the given Module with locking, ensuring that Module::process() is not called simultaneously.
+	Read-locks.
 	*/
 	json_t* moduleToJson(Module* module);
+	/** Serializes the given Module with locking, ensuring that Module::process() is not called simultaneously.
+	Write-locks.
+	*/
 	void moduleFromJson(Module* module, json_t* rootJ);
 
 	// Cables
 	size_t getNumCables();
+	/** Fills `cableIds` with up to `len` cable IDs in the rack.
+	Returns the number of IDs written.
+	This C-like method does no allocations. The vector C++ version below does.
+	Read-locks.
+	*/
 	size_t getCableIds(int64_t* cableIds, size_t len);
+	/** Returns a vector of cable IDs in the rack.
+	Read-locks.
+	*/
 	std::vector<int64_t> getCableIds();
-	/** Adds a cable to the rack engine.
+	/** Adds a Cable to the rack.
 	The cable ID must not be taken by another cable.
 	If the cable ID is -1, an ID is automatically assigned.
 	Does not transfer pointer ownership.
+	Write-locks.
 	*/
 	void addCable(Cable* cable);
+	/** Removes a Cable from the rack.
+	Write-locks.
+	*/
 	void removeCable(Cable* cable);
+	PRIVATE void removeCable_NoLock(Cable* cable);
+	/** Checks whether a Cable is in the rack.
+	Read-locks.
+	*/
+	bool hasCable(Cable* cable);
+	/** Returns the Cable with the given ID in the rack.
+	Read-locks.
+	*/
 	Cable* getCable(int64_t cableId);
 
 	// Params
@@ -122,19 +181,37 @@ struct Engine {
 	float getSmoothParam(Module* module, int paramId);
 
 	// ParamHandles
+	/** Adds a ParamHandle to the rack.
+	Does not automatically update the ParamHandle.
+	Write-locks.
+	*/
 	void addParamHandle(ParamHandle* paramHandle);
+	/**
+	Write-locks.
+	*/
 	void removeParamHandle(ParamHandle* paramHandle);
+	PRIVATE void removeParamHandle_NoLock(ParamHandle* paramHandle);
 	/** Returns the unique ParamHandle for the given paramId
+	Read-locks.
 	*/
 	ParamHandle* getParamHandle(int64_t moduleId, int paramId);
-	/** Use getParamHandle(moduleId, paramId) instead. */
+	/** Use getParamHandle(moduleId, paramId) instead.
+	Read-locks.
+	*/
 	DEPRECATED ParamHandle* getParamHandle(Module* module, int paramId);
 	/** Sets the ParamHandle IDs and module pointer.
 	If `overwrite` is true and another ParamHandle points to the same param, unsets that one and replaces it with the given handle.
+	Read-locks.
 	*/
 	void updateParamHandle(ParamHandle* paramHandle, int64_t moduleId, int paramId, bool overwrite = true);
 
+	/** Serializes the rack.
+	Read-locks.
+	*/
 	json_t* toJson();
+	/** Deserializes the rack.
+	Write-locks.
+	*/
 	void fromJson(json_t* rootJ);
 };
 

include/engine/Module.hpp

@@ -344,7 +344,8 @@ struct Module {
 	/** DEPRECATED. Override `onSampleRateChange(e)` instead. */
 	virtual void onSampleRateChange() {}
 
-	PRIVATE bool& bypass();
+	bool isBypass();
+	PRIVATE void setBypass(bool bypass);
 	PRIVATE const float* meterBuffer();
 	PRIVATE int meterLength();
 	PRIVATE int meterIndex();

include/rack.hpp

@@ -100,7 +100,7 @@
 
 
 #undef PRIVATE
-#define PRIVATE __attribute__((warning ("Using private function or symbol")))
+#define PRIVATE __attribute__((error ("Using private function or symbol")))
 
 
 namespace rack {

src/app/ModuleWidget.cpp

@@ -394,7 +394,7 @@ ModuleWidget::~ModuleWidget() {
 void ModuleWidget::draw(const DrawArgs& args) {
 	nvgScissor(args.vg, RECT_ARGS(args.clipBox));
 
-	if (module && module->bypass()) {
+	if (module && module->isBypass()) {
 		nvgGlobalAlpha(args.vg, 0.33);
 	}
 
@@ -1002,12 +1002,12 @@ void ModuleWidget::cloneAction() {
 
 void ModuleWidget::bypassAction() {
 	assert(module);
-	APP->engine->bypassModule(module, !module->bypass());
+	APP->engine->bypassModule(module, !module->isBypass());
 
 	// history::ModuleBypass
 	history::ModuleBypass* h = new history::ModuleBypass;
 	h->moduleId = module->id;
-	h->bypass = module->bypass();
+	h->bypass = module->isBypass();
 	APP->history->push(h);
 }
 
@@ -1075,7 +1075,7 @@ void ModuleWidget::createContextMenu() {
 	ModuleBypassItem* bypassItem = new ModuleBypassItem;
 	bypassItem->text = "Bypass";
 	bypassItem->rightText = RACK_MOD_CTRL_NAME "+E";
-	if (module && module->bypass())
+	if (module && module->isBypass())
 		bypassItem->rightText = CHECKMARK_STRING " " + bypassItem->rightText;
 	bypassItem->moduleWidget = this;
 	menu->addChild(bypassItem);

src/engine/Engine.cpp

@@ -32,15 +32,11 @@ static void initMXCSR() {
 
 
 /** Allows multiple "reader" threads to obtain a lock simultaneously, but only one "writer" thread.
-WriteLock can be used recursively.
-Uses reader priority, which implies that ReadLock can also be used recursively.
-This implementation is just a wrapper for pthreads.
+This implementation is currently just a wrapper for pthreads, which works on Linux/Mac/.
 This is available in C++17 as std::shared_mutex, but unfortunately we're using C++11.
 */
 struct ReadWriteMutex {
 	pthread_rwlock_t rwlock;
-	std::atomic<pthread_t> writerThread{0};
-	int recursion = 0;
 
 	ReadWriteMutex() {
 		if (pthread_rwlock_init(&rwlock, NULL))
@@ -58,21 +54,12 @@ struct ReadWriteMutex {
 			throw Exception("pthread_rwlock_unlock failed");
 	}
 	void lockWriter() {
-		pthread_t self = pthread_self();
-		if (writerThread != self) {
-			if (pthread_rwlock_wrlock(&rwlock))
-				throw Exception("pthread_rwlock_wrlock failed");
-			writerThread = self;
-		}
-		// We're safe behind a lock so we can increment the recursion count.
-		recursion++;
+		if (pthread_rwlock_wrlock(&rwlock))
+			throw Exception("pthread_rwlock_wrlock failed");
 	}
 	void unlockWriter() {
-		if (--recursion == 0) {
-			writerThread = 0;
-			if (pthread_rwlock_unlock(&rwlock))
-				throw Exception("pthread_rwlock_unlock failed");
-		}
+		if (pthread_rwlock_unlock(&rwlock))
+			throw Exception("pthread_rwlock_unlock failed");
 	}
 };
 
@@ -210,9 +197,6 @@ struct EngineWorker {
 };
 
 
-static const float FALLBACK_SAMPLE_RATE = 44100;
-
-
 struct Engine::Internal {
 	std::vector<Module*> modules;
 	std::vector<Cable*> cables;
@@ -240,7 +224,7 @@ struct Engine::Internal {
 	float smoothValue = 0.f;
 
 	/** Mutex that guards the Engine state, such as settings, Modules, and Cables.
-	Writers lock when mutating the engine's state.
+	Writers lock when mutating the engine's state or stepping the block.
 	Readers lock when using the engine's state.
 	*/
 	ReadWriteMutex mutex;
@@ -483,7 +467,7 @@ Engine::Engine() {
 	internal = new Internal;
 
 	internal->context = contextGet();
-	setSampleRate(FALLBACK_SAMPLE_RATE);
+	setSuggestedSampleRate(0.f);
 }
 
 
@@ -507,21 +491,25 @@ Engine::~Engine() {
 
 void Engine::clear() {
 	WriteLock lock(internal->mutex);
+	clear_NoLock();
+}
+
 
+void Engine::clear_NoLock() {
 	// Copy lists because we'll be removing while iterating
 	std::set<ParamHandle*> paramHandles = internal->paramHandles;
 	for (ParamHandle* paramHandle : paramHandles) {
-		removeParamHandle(paramHandle);
-		// Don't delete paramHandle because they're owned by other things (e.g. Modules)
+		removeParamHandle_NoLock(paramHandle);
+		// Don't delete paramHandle because they're normally owned by Module subclasses
 	}
 	std::vector<Cable*> cables = internal->cables;
 	for (Cable* cable : cables) {
-		removeCable(cable);
+		removeCable_NoLock(cable);
 		delete cable;
 	}
 	std::vector<Module*> modules = internal->modules;
 	for (Module* module : modules) {
-		removeModule(module);
+		removeModule_NoLock(module);
 		delete module;
 	}
 }
@@ -604,7 +592,8 @@ void Engine::setSuggestedSampleRate(float suggestedSampleRate) {
 		setSampleRate(suggestedSampleRate);
 	}
 	else {
-		setSampleRate(FALLBACK_SAMPLE_RATE);
+		// Fallback sample rate
+		setSampleRate(44100.f);
 	}
 }
 
@@ -711,6 +700,11 @@ void Engine::addModule(Module* module) {
 
 void Engine::removeModule(Module* module) {
 	WriteLock lock(internal->mutex);
+	removeModule_NoLock(module);
+}
+
+
+void Engine::removeModule_NoLock(Module* module) {
 	assert(module);
 	// Check that the module actually exists
 	auto it = std::find(internal->modules.begin(), internal->modules.end(), module);
@@ -792,7 +786,7 @@ void Engine::bypassModule(Module* module, bool bypass) {
 	WriteLock lock(internal->mutex);
 	assert(module);
 
-	if (module->bypass() == bypass)
+	if (module->isBypass() == bypass)
 		return;
 	// Clear outputs and set to 1 channel
 	for (Output& output : module->outputs) {
@@ -800,7 +794,7 @@ void Engine::bypassModule(Module* module, bool bypass) {
 		output.setChannels(0);
 	}
 	// Set bypass state
-	module->bypass() = bypass;
+	module->setBypass(bypass);
 	// Trigger event
 	if (bypass) {
 		Module::BypassEvent eBypass;
@@ -901,6 +895,11 @@ void Engine::addCable(Cable* cable) {
 
 void Engine::removeCable(Cable* cable) {
 	WriteLock lock(internal->mutex);
+	removeCable_NoLock(cable);
+}
+
+
+void Engine::removeCable_NoLock(Cable* cable) {
 	assert(cable);
 	// Check that the cable is already added
 	auto it = std::find(internal->cables.begin(), internal->cables.end(), cable);
@@ -935,6 +934,14 @@ void Engine::removeCable(Cable* cable) {
 }
 
 
+bool Engine::hasCable(Cable* cable) {
+	ReadLock lock(internal->mutex);
+	// TODO Performance could be improved by searching cablesCache, but more testing would be needed to make sure it's always valid.
+	auto it = std::find(internal->cables.begin(), internal->cables.end(), cable);
+	return it != internal->cables.end();
+}
+
+
 Cable* Engine::getCable(int64_t cableId) {
 	ReadLock lock(internal->mutex);
 	auto it = internal->cablesCache.find(cableId);
@@ -996,6 +1003,11 @@ void Engine::addParamHandle(ParamHandle* paramHandle) {
 
 void Engine::removeParamHandle(ParamHandle* paramHandle) {
 	WriteLock lock(internal->mutex);
+	removeParamHandle_NoLock(paramHandle);
+}
+
+
+void Engine::removeParamHandle_NoLock(ParamHandle* paramHandle) {
 	// Check that the ParamHandle is already added
 	auto it = internal->paramHandles.find(paramHandle);
 	assert(it != internal->paramHandles.end());
@@ -1035,6 +1047,7 @@ void Engine::updateParamHandle(ParamHandle* paramHandle, int64_t moduleId, int p
 
 	if (paramHandle->moduleId >= 0) {
 		// Replace old ParamHandle, or reset the current ParamHandle
+		// TODO Maybe call getParamHandle_NoLock()?
 		ParamHandle* oldParamHandle = getParamHandle(moduleId, paramId);
 		if (oldParamHandle) {
 			if (overwrite) {
@@ -1052,6 +1065,7 @@ void Engine::updateParamHandle(ParamHandle* paramHandle, int64_t moduleId, int p
 
 	// Set module pointer if the above block didn't reset it
 	if (paramHandle->moduleId >= 0) {
+		// TODO Maybe call getModule_NoLock()?
 		paramHandle->module = getModule(paramHandle->moduleId);
 	}
 
@@ -1086,8 +1100,9 @@ json_t* Engine::toJson() {
 
 
 void Engine::fromJson(json_t* rootJ) {
-	// Don't write-lock here because most of this function doesn't need it.
+	// Don't write-lock the entire method because most of it doesn't need it.
 
+	// Write-locks
 	clear();
 	// modules
 	json_t* modulesJ = json_object_get(rootJ, "modules");
@@ -1108,7 +1123,7 @@ void Engine::fromJson(json_t* rootJ) {
 				module->id = moduleIndex;
 			}
 
-			// This write-locks
+			// Write-locks
 			addModule(module);
 		}
 		catch (Exception& e) {
@@ -1131,7 +1146,7 @@ void Engine::fromJson(json_t* rootJ) {
 		Cable* cable = new Cable;
 		try {
 			cable->fromJson(cableJ);
-			// This write-locks
+			// Write-locks
 			addCable(cable);
 		}
 		catch (Exception& e) {

src/engine/Module.cpp

@@ -282,11 +282,16 @@ void Module::onRandomize(const RandomizeEvent& e) {
 }
 
 
-bool& Module::bypass() {
+bool Module::isBypass() {
 	return internal->bypass;
 }
 
 
+void Module::setBypass(bool bypass) {
+	internal->bypass = bypass;
+}
+
+
 const float* Module::meterBuffer() {
 	return internal->meterBuffer;
 }