Fix Python ownership detection and stabilize GIL handling for binding…#652
Merged
Merged
Conversation
Reviewer's GuideIntroduces deterministic Python interpreter ownership via an environment variable and adjusts the context manager to respect external interpreters and existing initialization while stabilizing GIL-related behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Looks IMHO very good |
rainman110
approved these changes
May 7, 2026
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider guarding
ensurePythonOwnerEnvVar()against cases whereQCoreApplicationis not yet constructed orapplicationFilePath()is empty, to avoid relying on an undefined or empty executable name when setting the default owner. - In
isGtlabPythonOwner(), it might be useful to handle unexpectedGTLAB_PYTHON_INTERPRETER_OWNERvalues (e.g., log or fall back explicitly) so misconfigured environments don’t silently default to one ownership mode based only on a string comparison.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider guarding `ensurePythonOwnerEnvVar()` against cases where `QCoreApplication` is not yet constructed or `applicationFilePath()` is empty, to avoid relying on an undefined or empty executable name when setting the default owner.
- In `isGtlabPythonOwner()`, it might be useful to handle unexpected `GTLAB_PYTHON_INTERPRETER_OWNER` values (e.g., log or fall back explicitly) so misconfigured environments don’t silently default to one ownership mode based only on a string comparison.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a deadlock in the interaction between the Python module, GTlabConsole, and the Python binding.
Previously, interpreter ownership was inferred from
Py_IsInitialized(), which could be incorrect when Python had already been initialized by another module. This led to deadlocks during context creation in CLI mode.Ownership is now controlled via
GTLAB_PYTHON_INTERPRETER_OWNER(gtlab/external), with automatic fallback initialization based on the current application name. This makes ownership detection deterministic and stabilizes GIL handling across both GTlabConsole and the Python binding.Summary by Sourcery
Stabilize Python interpreter ownership detection and GIL handling by making interpreter ownership explicit and environment-driven.
Bug Fixes:
Enhancements: