Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash from concurrent nb::make_iterator<> under free-threading. #832

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

hawkinsp
Copy link
Contributor

@hawkinsp hawkinsp commented Dec 23, 2024

The State class used by make_iterator is constructed lazily, but without locking it is possible for the caller to crash when the class type is only partially constructed. This PR adds an ft_mutex around the binding of the State type.

My initial instinct was to use an nb_object_guard on scope. Unfortunately this doesn't work; I suspect PyEval_SaveThread() is called during class binding and that releases the outer critical section.

@hawkinsp hawkinsp changed the title Fix crash from concurrent when nb::make_iterator<> under free-threading. Fix crash from concurrent nb::make_iterator<> under free-threading. Dec 23, 2024
The `State` class used by make_iterator is constructed lazily, but
without locking it is possible for the caller to crash when the class
type is only partially constructed. This PR adds an ft_mutex around the
binding of the State type.

My initial instinct was to use an nb_object_guard on `scope`.
Unfortunately this doesn't work; I suspect PyEval_SaveThread() is called
during class binding and that releases the outer critical section.
@wjakob
Copy link
Owner

wjakob commented Jan 6, 2025

This change looks good -- however, can you lock some member of internals instead of using a static function-local variable? These involve additional locking added by the compiler, which is not wanted here.

@hawkinsp
Copy link
Contributor Author

hawkinsp commented Jan 6, 2025

This change looks good -- however, can you lock some member of internals instead of using a static function-local variable? These involve additional locking added by the compiler, which is not wanted here.

No, it doesn't. A function-local static with a constant initializer does not need locking. See: https://godbolt.org/z/EKjMfW555

It's also not clear to me that any of the existing mutexes in internals are ok to use here: we're going to hold this lock while calling into the class binding code, which may acquire any of the remaining locks. If you'd like, I can add a new lock to nb_internals specifically for this purpose.

WDYT?

@wjakob
Copy link
Owner

wjakob commented Jan 7, 2025

That makes sense then. If forgot about the trivial nature of the constructor. Thanks for catching this!

@wjakob wjakob merged commit 27ba245 into wjakob:master Jan 7, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants