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

Various fixes to edge cases with GILGuard #1036

Merged
merged 1 commit into from
Aug 6, 2020

Conversation

davidhewitt
Copy link
Member

Closes #1019

I introduce panics for the two "nasty" cases discussed in that issue.

@davidhewitt davidhewitt force-pushed the fix-gilguard branch 3 times, most recently from 8a723f5 to b90e7c6 Compare July 14, 2020 20:43
@kngwyu
Copy link
Member

kngwyu commented Jul 15, 2020

Thank you!

  • For allow_threads
    I think it's OK to add T: Send.
    It's not so meaningful to return T: !Send in this context.

  • For nested case
    I'll leave some comments.

src/gil.rs Outdated Show resolved Hide resolved
src/gil.rs Outdated
// Safety check & update counter
GILGUARD_COUNT.with(|c| {
if c.get() - 1 != self.previous_count {
panic!("GILGuards must be dropped in the reverse of the order they are created.")
Copy link
Member

Choose a reason for hiding this comment

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

We should panic only when the gstate is PyGILState_UNLOCKED and GILGuard is nested.
In this nested case we care, gstate is always PyGILState_LOCKED and it's not problematic to set it in an incorrect order.

Copy link
Member

@kngwyu kngwyu Jul 15, 2020

Choose a reason for hiding this comment

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

What I meant is:

GIL_COUNT.try_with(|c| {
    if c.get() > 0 && self.gstate == PyGILState_UNLOCKED { panic!("...") }
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I'll go for that. Avoids needing to add GILGUARD_COUNT

@davidhewitt
Copy link
Member Author

Having looked further into these and subsequent tests, I've concluded that panic! is not the right solution for either of these two issues. In both cases, a panic will make it hard or impossible for user code to correct the GIL state, so Python will likely abort the process or deadlock shortly after the panic.

For allow_threads, I now agree we should add T: Send bound.

For the GILGuard drop out of order, I think the solution is less clear. I'm tempted to just add an abort() to this instead of panic, to be on the safe side.

@kngwyu
Copy link
Member

kngwyu commented Jul 20, 2020

so Python will likely abort the process or deadlock shortly after the panic.

  1. In extension case (we get GILGuard in #[pyfunction] or so), we don't need to worry about that so seriously. gstate is mostly LOCKED.
  2. In embedding case (we get GILGuard from main function or so), we don't need to recover the interpreter state, right?

@davidhewitt
Copy link
Member Author

It's not quite so clear-cut as that. Even for extensions, use of Python::allow_threads could lead to acquisition of new UNLOCKED GILGuards. Also, for the embedding case (~ tests) I was seeing the test process deadlock after a panic.

I have to say I really don't like the abort UX, so in the end I think I will keep panic! as the implementation. I'll add a whack of documentation to Python::acquire_gil to warn against possible issues here, and encourage users to use Python::with_gil.

Also, I think I'll change GILGuard::acquire to be just pub(crate), which will push users of it onto Python::acquire_gil. This means that all the safety notes can just go on Python::acquire_gil, as well as discouragement from using it!

@davidhewitt
Copy link
Member Author

I've pushed those discussed changes.

One interesting discovery from this was that PyErr is not Send. For now, I've pushed a couple of tweaks to this branch to make PyErr: Send. If you think this is a good idea, I'll first open a separate PR to make PyErr: Send properly. I think this would be a good idea anyway.

@kngwyu
Copy link
Member

kngwyu commented Jul 23, 2020

Thank you, though I don't understand the intention of some changes in the test files.

For now, I've pushed a couple of tweaks to this branch to make PyErr: Send. If you think this is a good idea, I'll first open a separate PR to make PyErr: Send properly. I think this would be a good idea anyway.

I agree that PyErr should be Send, but I'm sorry I can't understand what changes are related to PyErr.

@davidhewitt
Copy link
Member Author

I agree that PyErr should be Send, but I'm sorry I can't understand what changes are related to PyErr.

The example for Python::allow_threads returns PyResult<()>, and to make PyResult: Send, I had to make PyErr: Send. The alternative was to change the example, but I think the example is a good one.

Anyway, please hold on merging this PR; I'll make a new PR for PyErr: Send shortly.

I don't understand the intention of some changes in the test files.

Maybe the changes you are referring to are where I had to remove GILGuard::acquire usage from tests, because I've taken that function out of the PyO3 public API.

@davidhewitt
Copy link
Member Author

Having made the PyErr changes separately in #1067, I've rebased this PR and proceeding now to merge.

@davidhewitt davidhewitt merged commit 9823019 into PyO3:master Aug 6, 2020
@davidhewitt davidhewitt deleted the fix-gilguard branch December 24, 2021 02:05
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.

Is this „nasty“ code around GIL sound?
2 participants