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

Some more docs #2256

Merged
merged 3 commits into from
Mar 30, 2022
Merged

Some more docs #2256

merged 3 commits into from
Mar 30, 2022

Conversation

mejrs
Copy link
Member

@mejrs mejrs commented Mar 28, 2022

  • Use the new is method in some doc examples.
  • Clarifies some safety docs

@@ -882,6 +866,29 @@ impl<'py> Python<'py> {
}
}

impl<'unbound> Python<'unbound> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I've taken a liking to properly naming lifetimes as of recently. What would you think of renaming the Python<'py> lifetime to Python<'gil>? It's still short and I think it represents the meaning of the lifetime better.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the sentiment, but I am not sure if the improvement is worth the churn?

Copy link
Member

@davidhewitt davidhewitt Mar 30, 2022

Choose a reason for hiding this comment

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

It should be a fairly mechanical change if we wanted to update. If we were consistent with always having 'gil for the name of the GIL lifetime when present, that would also help make it clear when things did not depend on the GIL.

/// # Safety
///
/// - This token and any borrowed Python references derived from it can only be safely used
/// whilst the currently executing thread is actually holding the GIL.
Copy link
Member Author

@mejrs mejrs Mar 28, 2022

Choose a reason for hiding this comment

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

Re: threads, I was thinking about this:

fn foo(py: Python<'_>){
    let handle = std::thread::spawn(|| unsafe {
        let py = Python::assume_gil_acquired();
        // call some pyo3 apis
    });
    
    handle.join().unwrap();
}

Is this legal? Should we declare that it is not allowed?

Copy link
Member

@adamreichold adamreichold Mar 29, 2022

Choose a reason for hiding this comment

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

As spawn and join are synchronization points, I think this is legal as long as there is absolutely nothing happening in between spawn and join which could prevent the join from happening, i.e. the thread must be scoped.

However, I would suggest not including this example in the documentation as it opens more questions than it answers IMHO and I think the current wording is the right thing to do in almost all of the cases. When someone has to handle a case where this is not so (as in your example), they should be sure enough to figure it out. Everybody else is probably just confused by watering down the statement about the current thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

To clarify, the way I worded it would declare that it's not allowed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the above is legal - holding the GIL on the Python side also sets a bunch of thread-local variables about the interpreter state. Not holding the GIL in the exact thread the Python API is called in will likely lead to segfaults by reading invalid values from these variables.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Nice improvements, thanks!

Needs formatting fix, else I would have clicked merge ☺️

/// # Safety
///
/// - This token and any borrowed Python references derived from it can only be safely used
/// whilst the currently executing thread is actually holding the GIL.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the above is legal - holding the GIL on the Python side also sets a bunch of thread-local variables about the interpreter state. Not holding the GIL in the exact thread the Python API is called in will likely lead to segfaults by reading invalid values from these variables.

@@ -882,6 +866,29 @@ impl<'py> Python<'py> {
}
}

impl<'unbound> Python<'unbound> {
Copy link
Member

@davidhewitt davidhewitt Mar 30, 2022

Choose a reason for hiding this comment

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

It should be a fairly mechanical change if we wanted to update. If we were consistent with always having 'gil for the name of the GIL lifetime when present, that would also help make it clear when things did not depend on the GIL.

@mejrs mejrs merged commit 78efebd into PyO3:main Mar 30, 2022
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.

3 participants