Thread safety and cache integrity improvements

build.py

@@ -96,7 +96,7 @@ def release():
 					settings_path, next_version + snapshot_suffix,
 					'Bump version for next development iteration'
 				)
-				git('push', '-u', 'origin', 'master')
+				git('push', '-u', 'origin', 'main')
 				try:
 					git('push', 'origin', release_tag)
 					try:
@@ -106,7 +106,7 @@ def release():
 							'    git pull\n'
 							'    git checkout %s\n'
 							'    python build.py release\n'
-							'    git checkout master\n\n'
+							'    git checkout main\n\n'
 							'on the other OSs now, then come back here and do:'
 							'\n\n'
 							'    python build.py post_release\n'
@@ -117,7 +117,7 @@ def release():
 						raise
 				except:
 					git('revert', '--no-edit', revision_before + '..HEAD' )
-					git('push', '-u', 'origin', 'master')
+					git('push', '-u', 'origin', 'main')
 					revision_before = git('rev-parse', 'HEAD').rstrip()
 					raise
 			except:
@@ -149,7 +149,7 @@ def post_release():
 	create_cloudfront_invalidation(cloudfront_items_to_invalidate)
 	record_release_on_server()
 	upload_core_to_github()
-	git('checkout', 'master')
+	git('checkout', 'main')
 
 def _prompt_for_next_version(release_version):
 	next_version = _get_suggested_next_version(release_version)

src/main/python/fman/__init__.py

@@ -6,6 +6,7 @@
 from os import getenv
 from os.path import join, expanduser
 from PyQt5.QtWidgets import QMessageBox
+from threading import local as _thread_local
 
 import re
 
@@ -36,11 +37,16 @@
 PLATFORM = platform.name()
 
 if PLATFORM == 'Windows':
-	DATA_DIRECTORY = join(getenv('APPDATA'), 'fman')
+	_appdata = getenv('APPDATA')
+	if not _appdata:
+		raise RuntimeError('APPDATA environment variable is not set')
+	DATA_DIRECTORY = join(_appdata, 'fman')
 elif PLATFORM == 'Mac':
 	DATA_DIRECTORY = expanduser('~/Library/Application Support/fman')
 elif PLATFORM == 'Linux':
 	DATA_DIRECTORY = expanduser('~/.config/fman')
+else:
+	raise NotImplementedError('Unsupported platform: %s' % PLATFORM)
 
 class ApplicationCommand:
 	def __init__(self, window):
@@ -55,20 +61,20 @@ def aliases(self):
 def _set_path_onerror(e, url):
 	if isinstance(e, FileNotFoundError):
 		return dirname(url)
-	raise
+	raise e
 
 class DirectoryPane:
 	def __init__(self, window, widget, command_registry):
 		self.window = window
 		self._widget = widget
 		self._command_registry = command_registry
 		self._listeners = []
-		self._get_file_under_cursor_orig = self.get_file_under_cursor
+		self._file_under_cursor_override = _thread_local()
 
 	def _add_listener(self, listener):
 		self._listeners.append(listener)
 	def _broadcast(self, event, *args):
-		for listener in self._listeners:
+		for listener in list(self._listeners):
 			getattr(listener, event)(*args)
 
 	def get_commands(self):
@@ -77,7 +83,7 @@ def run_command(self, name, args=None):
 		if args is None:
 			args = {}
 		while True:
-			for listener in self._listeners:
+			for listener in list(self._listeners):
 				rewritten = listener.on_command(name, args)
 				if rewritten:
 					name, args = rewritten
@@ -98,6 +104,9 @@ def _remove_filter(self, filter_):
 	def get_selected_files(self):
 		return self._widget.get_selected_files()
 	def get_file_under_cursor(self):
+		override = getattr(self._file_under_cursor_override, 'value', None)
+		if override is not None:
+			return override
 		return self._widget.get_file_under_cursor()
 	def move_cursor_down(self, toggle_selection=False):
 		self._widget.move_cursor_down(toggle_selection)
@@ -113,14 +122,12 @@ def move_cursor_page_up(self, toggle_selection=False):
 		self._widget.move_cursor_page_up(toggle_selection)
 	def place_cursor_at(self, file_url):
 		self._widget.place_cursor_at(file_url)
-	# TODO: Rename to get_location()
 	def get_path(self):
 		return self._widget.get_location()
-	# TODO: Rename to set_location(...)
 	def set_path(self, dir_url, callback=None, onerror=_set_path_onerror):
 		args = dir_url, '', True
 		while True:
-			for listener in self._listeners:
+			for listener in list(self._listeners):
 				rewritten = listener.before_location_change(*args)
 				if rewritten and rewritten != args:
 					args = rewritten
@@ -154,9 +161,11 @@ def _has_focus(self):
 		return self._widget.hasFocus()
 	@contextmanager
 	def _override_file_under_cursor(self, value):
-		self.get_file_under_cursor = lambda: value
-		yield
-		self.get_file_under_cursor = self._get_file_under_cursor_orig
+		self._file_under_cursor_override.value = value
+		try:
+			yield
+		finally:
+			self._file_under_cursor_override.value = None
 
 class Window:
 	def __init__(self, widget, panecmd_registry):
@@ -197,10 +206,10 @@ def on_doubleclicked(self, file_url):
 		pass
 	def on_name_edited(self, file_url, new_name):
 		pass
-	# TODO: Rename to after_location_change()
 	def on_path_changed(self):
 		pass
 	def before_location_change(self, url, sort_column='', ascending=True):
+		"""Return (url, sort_column, ascending) to rewrite, or None to allow."""
 		pass
 	def on_files_dropped(self, file_urls, dest_dir, is_copy_not_move):
 		pass
@@ -273,7 +282,7 @@ def unload_plugin(plugin_path):
 
 class Task:
 
-	class Canceled(KeyboardInterrupt):
+	class Canceled(Exception):
 		pass
 
 	def __init__(self, title, size=0, fn=lambda: None, args=(), kwargs=None):

src/main/python/fman/fs.py

@@ -67,6 +67,9 @@ def notify_file_removed(url):
 	_get_mother_fs().notify_file_removed(url)
 
 class FileSystem:
+	"""Base class for file system plugins. Set scheme to eg. 'ftp://'.
+	Methods receive scheme-stripped paths, except copy/move which get full URLs.
+	"""
 
 	scheme = ''
 
@@ -108,7 +111,9 @@ def notify_file_added(self, path):
 	def notify_file_removed(self, path):
 		self._file_removed.trigger(self.scheme + path)
 	def notify_file_changed(self, path):
-		for callback in self._file_changed_callbacks.get(path, []):
+		with self._file_changed_callbacks_lock:
+			callbacks = list(self._file_changed_callbacks.get(path, []))
+		for callback in callbacks:
 			callback(self.scheme + path)
 	def samefile(self, path1, path2):
 		return self.resolve(path1) == self.resolve(path2)
@@ -146,11 +151,12 @@ def prepare_trash(self, path):
 			raise self._operation_not_implemented()
 		return [Task(
 			'Deleting ' + path.rsplit('/', 1)[-1],
-			fn=self.delete, args=(path,), size=1
+			fn=self.move_to_trash, args=(path,), size=1
 		)]
 	def touch(self, path):
 		raise self._operation_not_implemented()
 	def copy(self, src_url, dst_url):
+		"""Unlike other methods, receives full URLs (with scheme), not paths."""
 		raise self._operation_not_implemented()
 	def prepare_copy(self, src_url, dst_url):
 		if self.copy.__func__ is FileSystem.copy:
@@ -161,6 +167,7 @@ def prepare_copy(self, src_url, dst_url):
 			fn=self.copy, args=(src_url, dst_url)
 		)]
 	def move(self, src_url, dst_url):
+		"""Unlike other methods, receives full URLs (with scheme), not paths."""
 		raise self._operation_not_implemented()
 	def prepare_move(self, src_url, dst_url):
 		if self.move.__func__ is FileSystem.move:

src/main/python/fman/impl/fs_cache.py

@@ -1,37 +1,89 @@
-from collections import defaultdict
-from threading import Lock
+# Lock ordering (acquire top-to-bottom, never invert):
+#   Cache._lock
+#     CacheItem._children_lock (per-node, parent before child)
+#       CacheItem._attr_locks_lock (per-node)
+#         CacheItem attr RLock (per-attr)
+# Cache._lock must NOT be held when calling compute_value in query(),
+# because compute_value may trigger nested cache operations.
+from threading import Lock, RLock
 
 class Cache:
 	def __init__(self):
+		self._lock = Lock()
 		self._root = CacheItem()
+		self._generation = 0
 	def put(self, path, attr, value):
-		self._root.update_child(path).put(attr, value)
+		with self._lock:
+			self._root.update_child(path).put(attr, value)
 	def get(self, path, attr):
-		return self._root.get_child(path).get(attr)
+		with self._lock:
+			return self._root.get_child(path).get(attr)
 	def query(self, path, attr, compute_value):
-		return self._root.update_child(path).query(attr, compute_value)
+		while True:
+			with self._lock:
+				gen = self._generation
+				item = self._root.update_child(path)
+			result = item.query(attr, compute_value)
+			with self._lock:
+				if self._generation == gen:
+					return result
+			item.clear_attr(attr)
+	def mutate(self, path, attr, fn):
+		with self._lock:
+			try:
+				item = self._root.get_child(path)
+				value = item.get(attr)
+			except KeyError:
+				return
+			fn(value)
 	def clear(self, path):
-		if not path:
-			self._root = CacheItem()
-		else:
+		with self._lock:
+			self._generation += 1
+			if not path:
+				self._root = CacheItem()
+			else:
+				try:
+					self._root.delete_child(path)
+				except KeyError:
+					pass
+	def clear_attr(self, path, attr):
+		with self._lock:
 			try:
-				self._root.delete_child(path)
+				item = self._root.get_child(path)
 			except KeyError:
-				pass
+				return
+			item.clear_attr(attr)
 
 class CacheItem:
 	def __init__(self):
 		self._children = {}
+		self._children_lock = Lock()
 		self._attrs = {}
-		self._attr_locks = defaultdict(Lock)
+		self._attr_locks_lock = Lock()
+		self._attr_locks = {}
 	def put(self, attr, value):
-		self._attrs[attr] = value
+		with self._attr_locks_lock:
+			if attr not in self._attr_locks:
+				self._attr_locks[attr] = RLock()
+			lock = self._attr_locks[attr]
+		with lock:
+			self._attrs[attr] = value
 	def get(self, attr):
 		return self._attrs[attr]
+	def clear_attr(self, attr):
+		with self._attr_locks_lock:
+			lock = self._attr_locks.pop(attr, None)
+		if lock:
+			with lock:
+				self._attrs.pop(attr, None)
+		else:
+			self._attrs.pop(attr, None)
 	def query(self, attr, compute_value):
-		# Because `defaultdict` and `Lock` are implemented in C, they do not
-		# release the GIL and the dict access is atomic:
-		with self._attr_locks[attr]:
+		with self._attr_locks_lock:
+			if attr not in self._attr_locks:
+				self._attr_locks[attr] = RLock()
+			lock = self._attr_locks[attr]
+		with lock:
 			try:
 				return self._attrs[attr]
 			except KeyError:
@@ -57,4 +109,4 @@ def delete_child(self, path):
 		if len(parts) == 1:
 			del self._children[parts[0]]
 		else:
-			self._children[parts[0]].delete_child(parts[1])
+			self._children[parts[0]].delete_child(parts[1])

src/main/python/fman/impl/model/__init__.py

@@ -20,6 +20,8 @@ class SortedFileSystemModel(QSortFilterProxyModel):
 	sort_order_changed = pyqtSignal(int, int)
 	transaction_ended = pyqtSignal()
 
+	_MAX_VISITED = 512
+
 	def __init__(self, parent, fs, null_location):
 		super().__init__(parent)
 		self._fs = fs
@@ -105,6 +107,9 @@ def _set_location_main(
 		)
 		self.setSourceModel(new_model)
 		self._connect_signals(new_model)
+		if len(self._already_visited) > self._MAX_VISITED:
+			self._already_visited.clear()
+			self._already_visited.add(url)
 		self._already_visited.add(url)
 		self.location_changed.emit(url)
 		order = Qt.AscendingOrder if ascending else Qt.DescendingOrder
@@ -146,21 +151,19 @@ def url(self, index):
 	def find(self, url):
 		return self.mapFromSource(self.sourceModel().find(url))
 	def _on_file_removed(self, url):
+		if not self.sourceModel():
+			return
 		if is_pardir(url, self.get_location()):
-			dir_ = dirname(url)
-			if dir_ == url:
-				self.set_location(self._null_location)
-			else:
+			while True:
+				dir_ = dirname(url)
+				if dir_ == url:
+					self.set_location(self._null_location)
+					return
 				try:
 					self.set_location(dir_)
+					return
 				except OSError:
-					# In a perfect world, would like to only handle
-					# FileNotFoundError here. But there can of course also be
-					# other reasons. For example, when on a network share on
-					# Windows, we may get a PermissionError trying to list a
-					# parent directory we don't have access to. So catch all
-					# OSErrors and in the worst case go to null://.
-					self._on_file_removed(dir_)
+					url = dir_
 	def _connect_signals(self, model):
 		# Would prefer signal.connect(self.signal.emit) here. But PyQt doesn't
 		# support it. So we need Python wrappers "_emit_...":
@@ -189,5 +192,11 @@ def _emit_sort_order_changed(self, column, order):
 		self.sort_order_changed.emit(column, order)
 	def _emit_transaction_ended(self):
 		self.transaction_ended.emit()
+	def shutdown(self):
+		self._fs.file_removed.remove_callback(self._on_file_removed)
+		model = self.sourceModel()
+		if model:
+			self._disconnect_signals(model)
+			model.shutdown()
 	def __str__(self):
 		return '<%s: %s>' % (self.__class__.__name__, self.get_location())