resolved illegal threading issue; close #32 by iamtekson · Pull Request #38 · iamtekson/GeoAgent

iamtekson · 2026-01-05T04:49:28Z

No description provided.

Copilot

Pull request overview

This PR resolves an illegal threading issue (#32) by replacing the old callback-based threading mechanism with a new unified MainThreadRunner approach for executing QGIS operations safely on the main Qt thread.

Key changes:

Introduced MainThreadRunner class with BlockingQueuedConnection to replace multiple dispatcher classes
Added @qgis_main_thread decorator for seamless main-thread execution of tool functions
Removed specialized dispatchers (RefreshDispatcher, ProjectLoadDispatcher, LayerRemovalDispatcher) in favor of the unified runner

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
utils/project_loader.py	Deleted - specialized project loading dispatcher removed
utils/layer_operations.py	Deleted - specialized layer removal dispatcher removed
utils/canvas_refresh.py	Replaced old `RefreshDispatcher` with new `MainThreadRunner` class and `execute_on_main_thread` function; added `@qgis_main_thread` decorator
tools/io.py	Applied `@qgis_main_thread` decorator to all tool functions; replaced callback-based operations with direct QGIS API calls
tools/filters.py	Applied `@qgis_main_thread` decorator and replaced `refresh_map_canvas()` with direct canvas refresh
tools/geoprocessing.py	Removed TODO comment
tools/init.py	Changed from explicit imports to wildcard import for geoprocessing module
geo_agent.py	Removed old dispatcher initialization and callbacks; added `MainThreadRunner` initialization and global setup

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-05T04:53:04Z

+
+        # main logic to save project
+        try:
+            project.setFileName(path)
+            project.write(path)
+            _logger.info(f"Successfully created project at '{path}'")
+            return f"**Success:** Created new project at **{path}**"
+        except Exception as e:
+            _logger.error(f"Error saving project on main thread: {str(e)}", exc_info=True)
+            return f"**Error:** creating project: {str(e)}"
+


Nested exception handling creates redundant error paths. The inner try-except on lines 403-410 catches exceptions and returns an error message, but this return happens inside the outer try block (starting at line 377), so if the inner exception handler doesn't catch something, the outer handler on lines 412-414 will also catch it and return a different error message format. Consider removing the nested try-except and using a single exception handler for cleaner error handling.

Suggested change

# main logic to save project

try:

project.setFileName(path)

project.write(path)

_logger.info(f"Successfully created project at '{path}'")

return f"**Success:** Created new project at **{path}**"

except Exception as e:

_logger.error(f"Error saving project on main thread: {str(e)}", exc_info=True)

return f"**Error:** creating project: {str(e)}"

# main logic to save project

project.setFileName(path)

project.write(path)

_logger.info(f"Successfully created project at '{path}'")

return f"**Success:** Created new project at **{path}**"

Copilot · 2026-01-05T04:53:05Z

+    runner = _global_main_runner 
+
+    # This is the magic part: invokeMethod with BlockingQueuedConnection
+    QMetaObject.invokeMethod(
+        runner, 
+        "run_task", 
+        Qt.BlockingQueuedConnection,
+        Q_ARG(object, func),
+        Q_ARG(list, list(args)),
+        Q_ARG(dict, kwargs)
+    )
+


Missing null check for _global_main_runner. If execute_on_main_thread is called before set_main_runner is invoked (e.g., if a tool is called before the plugin is fully initialized), this will cause an AttributeError when trying to invoke methods on None. Add a validation check and raise an informative error message.

Suggested change

runner = _global_main_runner

# This is the magic part: invokeMethod with BlockingQueuedConnection

QMetaObject.invokeMethod(

runner,

"run_task",

Qt.BlockingQueuedConnection,

Q_ARG(object, func),

Q_ARG(list, list(args)),

Q_ARG(dict, kwargs)

)

runner = _global_main_runner

if runner is None:

raise RuntimeError(

"MainThreadRunner is not set. Call set_main_runner(...) during plugin "

"initialization before calling execute_on_main_thread."

)

# This is the magic part: invokeMethod with BlockingQueuedConnection

QMetaObject.invokeMethod(

runner,

"run_task",

Qt.BlockingQueuedConnection,

Q_ARG(object, func),

Q_ARG(list, list(args)),

Q_ARG(dict, kwargs)

)

Copilot · 2026-01-05T04:53:05Z

+            return {"success": success, "error": None if success else f"Failed to load project from '{path}'. The file may be corrupted or incompatible."}
+
+        except Exception as e:
+            _logger.error(f"Error loading project on main thread: {str(e)}", exc_info=True)
+            return {"success": False, "error": str(e)}


Inconsistent return type in load_qgis_project function. The function signature declares it returns str, but lines 506 and 510 return dict objects with 'success' and 'error' keys, while lines 492, 499, and 514 correctly return strings. This inconsistency will cause type errors for callers expecting a string return value. All return statements should return strings matching the function signature.

Copilot · 2026-01-05T04:53:05Z

-    find_processing_algorithm,
-)
+# for easy importing geoprocessing tools later
+from .geoprocessing import * 


Wildcard import using 'from .geoprocessing import *' is a bad practice as it makes it unclear which names are being imported and can lead to namespace pollution and naming conflicts. Explicitly import the required functions (execute_processing, list_processing_algorithms, get_algorithm_parameters, find_processing_algorithm) as was done in the removed code.

Suggested change

from .geoprocessing import *

from .geoprocessing import (

execute_processing,

list_processing_algorithms,

get_algorithm_parameters,

find_processing_algorithm,

)

Copilot · 2026-01-05T04:53:05Z

+    QMetaObject.invokeMethod(
+        runner, 
+        "run_task", 
+        Qt.BlockingQueuedConnection,
+        Q_ARG(object, func),
+        Q_ARG(list, list(args)),
+        Q_ARG(dict, kwargs)
+    )


Missing error handling for QMetaObject.invokeMethod. If the invocation fails (returns False), the code continues without any indication of failure. The return value of invokeMethod should be checked, and an appropriate error should be raised if it fails.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-01-05T04:53:06Z

+    @pyqtSlot(object, list, dict)
+    def run_task(self, func, args, kwargs):
+        """Internal slot to execute the function and store result."""
+        self._result = None
+        self._error = None
+        try:
+            self._result = func(*args, **kwargs)
+        except Exception as e:
+            self._error = e


Potential race condition with _result and _error attributes. These instance variables are accessed by both the main thread (in run_task) and worker threads (in execute_on_main_thread checking runner._error and runner._result). If multiple worker threads call execute_on_main_thread concurrently using the same runner instance, they could overwrite each other's results. Consider using thread-local storage or a different synchronization mechanism to ensure each call has isolated result/error storage.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-01-05T04:53:06Z

+        try:
+            project = QgsProject.instance()
+            project.clear()
+            success = project.read(path)
            _logger.info(f"Successfully loaded project from '{path}'")
-            return f"**Success:** Project loaded from **{path}**"
-        else:
-            _logger.error(f"Unknown error loading project from '{path}'")
-            return f"**Error:** Failed to load project from **{path}**."
+            return {"success": success, "error": None if success else f"Failed to load project from '{path}'. The file may be corrupted or incompatible."}
+
+        except Exception as e:
+            _logger.error(f"Error loading project on main thread: {str(e)}", exc_info=True)
+            return {"success": False, "error": str(e)}


Nested exception handling creates redundant error paths. The inner try-except on lines 501-510 catches exceptions and returns an error (either dict or string), but this return happens inside the outer try block (starting at line 489), so if the inner exception handler doesn't catch something, the outer handler on lines 512-514 will also catch it and return a different error format. Consider removing the nested try-except and using a single exception handler for cleaner error handling.

Copilot · 2026-01-05T04:53:06Z

+        # main logic to save project
+        try:
+            project.setFileName(path)
+            project.write(path)


Missing success check after project.write(). The write operation on line 405 returns a boolean indicating success/failure, but this return value is ignored. If the write fails, the function will still log success and return a success message. Store the return value and check if it's False before returning success.

Suggested change

project.write(path)

write_success = project.write(path)

if not write_success:

_logger.error(f"Failed to write QGIS project to '{path}'")

return f"**Error:** Failed to create project at **{path}**. Could not write project file."

Copilot · 2026-01-05T04:53:06Z

@@ -1,4 +1,4 @@
-from ..utils.canvas_refresh import refresh_map_canvas
+from ..utils.canvas_refresh import get_qgis_interface, qgis_main_thread, execute_on_main_thread


Import of 'execute_on_main_thread' is not used.

Suggested change

from ..utils.canvas_refresh import get_qgis_interface, qgis_main_thread, execute_on_main_thread

from ..utils.canvas_refresh import get_qgis_interface, qgis_main_thread

Copilot · 2026-01-05T04:53:07Z

-from ..utils.canvas_refresh import get_qgis_interface
-from ..utils.layer_operations import remove_layer_on_main_thread
-from ..utils.project_loader import load_project_on_main_thread
+from ..utils.canvas_refresh import get_qgis_interface, execute_on_main_thread, qgis_main_thread


Import of 'execute_on_main_thread' is not used.

Suggested change

from ..utils.canvas_refresh import get_qgis_interface, execute_on_main_thread, qgis_main_thread

from ..utils.canvas_refresh import get_qgis_interface, qgis_main_thread

Copilot · 2026-01-05T05:03:43Z

@iamtekson I've opened a new pull request, #39, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-01-05T05:04:47Z

@iamtekson I've opened a new pull request, #40, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Add error handling for QMetaObject.invokeMethod in thread-safe execution

Fix race condition in MainThreadRunner with thread-indexed result storage

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-05T05:27:25Z

+        raise RuntimeError("MainThreadRunner is not set. Please set it using set_main_runner().")
+
+    # Get the current thread ID to isolate results
+    thread_id = threading.get_ident()


Thread IDs can be reused by the operating system after a thread terminates. If a new thread gets the same ID as a previous thread whose result hasn't been popped yet, it could retrieve the wrong result. This is mitigated by the pop operation, but race conditions could still occur. Consider using a combination of thread ID and a sequence number or UUID for unique identification.

Copilot · 2026-01-05T05:27:25Z

+def qgis_main_thread(func):
+    """
+    Decorator that automatically wraps a function to run 
+    on the QGIS main thread using the Global Runner.
+    """
+    @wraps(func)
+    def wrapper(*args, **kwargs):
+        # We use the existing execute_on_main_thread logic
+        return execute_on_main_thread(func, *args, **kwargs)
+    return wrapper


The decorator silently changes the execution context of all decorated functions to run on the main thread. This could lead to unexpected blocking behavior, especially if a developer is unaware that a function decorated with @qgis_main_thread will block the calling thread. Consider adding a docstring or naming the decorator more explicitly (e.g., @blocking_main_thread) to make this behavior more obvious.

Copilot · 2026-01-05T05:27:25Z

-        self._layer_removal_dispatcher = LayerRemovalDispatcher(self.iface)
+
+        # main runner
+        self.main_runner = MainThreadRunner()


The MainThreadRunner instance is created in the constructor but Qt objects should typically be created on the thread where they will live (the main Qt thread). Creating this QObject in init which may be called from any thread could lead to threading issues. Consider moving the MainThreadRunner instantiation to the initGui() method which is guaranteed to run on the main thread.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-01-05T05:27:26Z

+        self._results = {}  # Dictionary to store results per thread ID
+        self._lock = threading.Lock()  # Lock to protect the results dictionary
+
+    @pyqtSlot(object, list, dict, int)
+    def run_task(self, func, args, kwargs, thread_id):
+        """Internal slot to execute the function and store result."""
+        result = None
+        error = None
+        try:
+            result = func(*args, **kwargs)
+        except Exception as e:
+            error = e
+
+        # Store the result/error for this specific thread
+        with self._lock:
+            self._results[thread_id] = (result, error)


Potential memory leak: results stored in _results dictionary are only cleaned up when popped in execute_on_main_thread. If a thread ID is used but the result is never retrieved (e.g., due to an exception before the pop), the result will remain in the dictionary indefinitely. Consider implementing a cleanup mechanism or using WeakValueDictionary.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-01-05T05:31:27Z

@iamtekson I've opened a new pull request, #41, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-01-05T05:32:02Z

@iamtekson I've opened a new pull request, #42, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Fix Qt threading violation in MainThreadRunner initialization

Fix memory leak in MainThreadRunner result cleanup

resolved illegal threading issue; close #32

64597e8

iamtekson requested a review from Copilot January 5, 2026 04:49

Copilot started reviewing on behalf of iamtekson January 5, 2026 04:49 View session

iamtekson added 2 commits January 4, 2026 21:51

v0.2.0

1d7457b

Merge branch 'main' of github.com:iamtekson/GeoAgent into dev

451338a

Copilot AI reviewed Jan 5, 2026

View reviewed changes

iamtekson and others added 2 commits January 4, 2026 22:01

logger in filter functions

b1650cc

Initial plan

94f8641

Copilot AI mentioned this pull request Jan 5, 2026

Add error handling for QMetaObject.invokeMethod in thread-safe execution #39

Merged

Initial plan

4034bc7

Copilot AI mentioned this pull request Jan 5, 2026

Fix race condition in MainThreadRunner with thread-indexed result storage #40

Merged

Copilot AI and others added 8 commits January 5, 2026 05:06

Add error handling for QMetaObject.invokeMethod return value

8ea1269

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

remove unnecessary imports and try/except block

f0dd0ef

Fix race condition in MainThreadRunner using thread-safe storage

085cca3

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Add explicit check for thread result existence

5d37da7

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Merge pull request #39 from iamtekson/copilot/sub-pr-38

395a329

Add error handling for QMetaObject.invokeMethod in thread-safe execution

Merge branch 'dev' into copilot/sub-pr-38-again

f1c71c2

Merge pull request #40 from iamtekson/copilot/sub-pr-38-again

760a63b

Fix race condition in MainThreadRunner with thread-indexed result storage

resolve multi-threading issue

5785214

iamtekson requested a review from Copilot January 5, 2026 05:24

Copilot started reviewing on behalf of iamtekson January 5, 2026 05:24 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

Initial plan

76f60ae

Copilot AI mentioned this pull request Jan 5, 2026

Fix Qt threading violation in MainThreadRunner initialization #41

Merged

Initial plan

ba7e60d

Copilot AI mentioned this pull request Jan 5, 2026

Fix memory leak in MainThreadRunner result cleanup #42

Merged

iamtekson and others added 14 commits January 4, 2026 22:32

Update utils/canvas_refresh.py

2774e53

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tools/io.py

5cb9511

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Move MainThreadRunner instantiation to initGui() method

1ac7468

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Add defensive check for MainThreadRunner in run() method

7ca9199

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Fix memory leak in MainThreadRunner by adding try-finally cleanup

7f32ff3

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Address code review feedback: update comments and add warning

bcb107b

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Improve comments and use single quotes for consistency

c712ff1

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Refactor cleanup to use single pop in finally block

1dbe844

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Improve documentation for MainThreadRunner setup

8cca0eb

Co-authored-by: iamtekson <39838116+iamtekson@users.noreply.github.com>

Merge branch 'dev' into copilot/sub-pr-38

8c6657d

Merge pull request #41 from iamtekson/copilot/sub-pr-38

ec61662

Fix Qt threading violation in MainThreadRunner initialization

Merge branch 'dev' into copilot/sub-pr-38-again

d16b385

Merge pull request #42 from iamtekson/copilot/sub-pr-38-again

6546d00

Fix memory leak in MainThreadRunner result cleanup

modified UI main page

626234e

iamtekson merged commit 975a69a into main Jan 5, 2026

iamtekson deleted the dev branch January 5, 2026 05:47

-from .geoprocessing import *
+from .geoprocessing import (
+    execute_processing,
+    list_processing_algorithms,
+    get_algorithm_parameters,
+    find_processing_algorithm,
+)

-            project.write(path)
+            write_success = project.write(path)
+            if not write_success:
+                _logger.error(f"Failed to write QGIS project to '{path}'")
+                return f"**Error:** Failed to create project at **{path}**. Could not write project file."

		@@ -1,4 +1,4 @@
		from ..utils.canvas_refresh import refresh_map_canvas
		from ..utils.canvas_refresh import get_qgis_interface, qgis_main_thread, execute_on_main_thread

Conversation

iamtekson commented Jan 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

iamtekson Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

iamtekson Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Jan 5, 2026

Uh oh!

Copilot AI commented Jan 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

iamtekson Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

iamtekson Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI commented Jan 5, 2026

Uh oh!

Copilot AI commented Jan 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects