Merge pull request #1067 from TomHarte/MachineLeak

macOS: Avoid likely leak of machines.

Concurrency/AsyncTaskQueue.hpp

@@ -73,7 +73,7 @@ template <bool perform_automatically, typename Performer = int> class TaskQueue:
 				[this] {
 					ActionVector actions;
 
-					while(!should_quit) {
+					while(!should_quit_) {
 						// Wait for new actions to be signalled, and grab them.
 						std::unique_lock lock(condition_mutex_);
 						while(actions_.empty()) {
@@ -127,7 +127,7 @@ template <bool perform_automatically, typename Performer = int> class TaskQueue:
 		/// The queue cannot be restarted; this is a destructive action.
 		void stop() {
 			if(thread_.joinable()) {
-				should_quit = true;
+				should_quit_ = true;
 				enqueue([] {});
 				if constexpr (!perform_automatically) {
 					perform();
@@ -168,7 +168,7 @@ template <bool perform_automatically, typename Performer = int> class TaskQueue:
 		ActionVector actions_;
 
 		// Necessary synchronisation parts.
-		std::atomic<bool> should_quit = false;
+		std::atomic<bool> should_quit_ = false;
 		std::mutex condition_mutex_;
 		std::condition_variable condition_;
 

OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m

@@ -47,7 +47,11 @@
 
 	if(self) {
 		_deallocLock = [[NSLock alloc] init];
+		_deallocLock.name = @"Dealloc lock";
+
 		_queueLock = [[NSLock alloc] init];
+		_queueLock.name = @"Audio queue access lock";
+
 		atomic_store_explicit(&_enqueuedBuffers, 0, memory_order_relaxed);
 
 		_samplingRate = samplingRate;
@@ -115,11 +119,12 @@
 
 - (void)dealloc {
 	[_deallocLock lock];
+		// Ensure no buffers remain enqueued by stopping the queue.
 		if(_audioQueue) {
-			OSSGuard(AudioQueueDispose(_audioQueue, true));
+			OSSGuard(AudioQueueStop(_audioQueue, true));
-			_audioQueue = NULL;
 		}
 
+		// Free all buffers.
 		for(size_t c = 0; c < NumBuffers; c++) {
 			if(_buffers[c]) {
 				OSSGuard(AudioQueueFreeBuffer(_audioQueue, _buffers[c]));
@@ -127,6 +132,12 @@
 			}
 		}
 
+		// Dispose of the queue.
+		if(_audioQueue) {
+			OSSGuard(AudioQueueDispose(_audioQueue, true));
+			_audioQueue = NULL;
+		}
+
 		// nil out the dealloc lock before entering the critical section such
 		// that it becomes impossible for anyone else to acquire.
 		NSLock *deallocLock = _deallocLock;

OSBindings/Mac/Clock Signal/Machine/CSMachine.mm

@@ -669,41 +669,57 @@ struct ActivityObserver: public Activity::Observer {
 #pragma mark - Timer
 
 - (void)audioQueueIsRunningDry:(nonnull CSAudioQueue *)audioQueue {
-	updater.enqueue([self] {
+	__weak CSMachine *weakSelf = self;
-		updater.performer.machine->flush_output(MachineTypes::TimedMachine::Output::Audio);
+
+	updater.enqueue([weakSelf] {
+		CSMachine *const strongSelf = weakSelf;
+		if(strongSelf) {
+			strongSelf->updater.performer.machine->flush_output(MachineTypes::TimedMachine::Output::Audio);
+		}
 	});
 }
 
 - (void)scanTargetViewDisplayLinkDidFire:(CSScanTargetView *)view now:(const CVTimeStamp *)now outputTime:(const CVTimeStamp *)outputTime {
-	updater.enqueue([self] {
+	__weak CSMachine *weakSelf = self;
+
+	updater.enqueue([weakSelf] {
+		CSMachine *const strongSelf = weakSelf;
+		if(!strongSelf) {
+			return;
+		}
+
 		// Grab a pointer to the timed machine from somewhere where it has already
 		// been dynamically cast, to avoid that cost here.
-		MachineTypes::TimedMachine *const timed_machine = updater.performer.machine;
+		MachineTypes::TimedMachine *const timed_machine = strongSelf->updater.performer.machine;
 
 		// Definitely update video; update audio too if that pipeline is looking a little dry.
 		auto outputs = MachineTypes::TimedMachine::Output::Video;
-		if(_audioQueue.isRunningDry) {
+		if(strongSelf->_audioQueue.isRunningDry) {
 			outputs |= MachineTypes::TimedMachine::Output::Audio;
 		}
 		timed_machine->flush_output(outputs);
 
 		// Attempt sync-matching if this machine is a fit.
-		//
+		const auto scanStatus = strongSelf->_machine->scan_producer()->get_scan_status();
-		// TODO: either cache scan_producer(), or possibly start caching these things
+		const bool canSynchronise = strongSelf->_scanSynchroniser.can_synchronise(
-		// inside the DynamicMachine?
+			scanStatus,
-		const auto scanStatus = self->_machine->scan_producer()->get_scan_status();
+			strongSelf.view.refreshPeriod
-		const bool canSynchronise = self->_scanSynchroniser.can_synchronise(scanStatus, self.view.refreshPeriod);
+		);
+
 		if(canSynchronise) {
-			const double multiplier = self->_scanSynchroniser.next_speed_multiplier(self->_machine->scan_producer()->get_scan_status());
+			const double multiplier = strongSelf->_scanSynchroniser.next_speed_multiplier(
+				strongSelf->_machine->scan_producer()->get_scan_status()
+			);
 			timed_machine->set_speed_multiplier(multiplier);
 		} else {
 			timed_machine->set_speed_multiplier(1.0);
 		}
 
 		// Ask Metal to rasterise all that just happened and present it.
-		[self.view updateBacking];
+		[strongSelf.view updateBacking];
 		dispatch_async(dispatch_get_main_queue(), ^{
-			[self.view draw];
+			// This is safe even if weakSelf has been nulled out in the interim.
+			[weakSelf.view draw];
 		});
 	});
 }