Skip to content

Conversation

@sgonorov
Copy link
Contributor

Fix update from release branch

@sgonorov sgonorov self-assigned this Nov 25, 2025
@github-actions github-actions bot added the category: Python API Python API for GenAI label Nov 25, 2025
@sgonorov sgonorov force-pushed the fix_deadlock_update_from_release branch from 4b7cd4f to 6d3fa90 Compare November 26, 2025 12:56
@Wovchena Wovchena requested a review from Copilot November 26, 2025 13:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a deadlock issue in Python callbacks by ensuring proper GIL (Global Interpreter Lock) management during callback lifecycle. The fix addresses the problem of Python objects being destroyed without holding the GIL, which can cause deadlocks in multi-threaded scenarios.

Key changes:

  • Updated callback signature to use py::str instead of std::string for better Python integration
  • Added custom deleters to shared pointers that acquire GIL before destroying Python objects
  • Improved error handling in UTF-8 string decoding with null pointer checks

Reviewed changes

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

File Description
src/python/py_utils.hpp Changed callback signature from std::string to py::str parameter
src/python/py_utils.cpp Added GIL-aware custom deleters for Python callbacks and improved UTF-8 decoding error handling

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

if (Py_IsInitialized()) {
PyGILState_STATE gstate = PyGILState_Ensure();
delete f;
PyGILState_Release(gstate);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the results of running tests without acquired GIL? Do the really fail and this code is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if GIL is not there it means we have undefined behaviour, it fails from time to time.

if (Py_IsInitialized()) {
PyGILState_STATE gstate = PyGILState_Ensure();
delete f;
PyGILState_Release(gstate);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not gil_scoped_acquire?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced it with gil_scoped_acquire - I've tried using C-api to check how it works. But looks like it doesn't make any difference.

@sgonorov sgonorov force-pushed the fix_deadlock_update_from_release branch from c83d010 to 4ffbb64 Compare November 27, 2025 07:29
@sgonorov sgonorov force-pushed the fix_deadlock_update_from_release branch from 4ffbb64 to 06d0693 Compare November 27, 2025 22:02
@github-actions github-actions bot added category: image generation Image generation pipelines category: GHA CI based on Github actions category: GGUF GGUF file reader labels Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: GGUF GGUF file reader category: GHA CI based on Github actions category: image generation Image generation pipelines category: Python API Python API for GenAI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants