Skip to content

Conversation

ntjohnson1
Copy link
Contributor

Which issue does this PR close?

Happy to write a more focussed issue since this is just specific to SessionContext.

Closes #366

Rationale for this change

The issues provides a minimal repro for my motivating use case.

What changes are included in this PR?

Makes SessionContext frozen since all the methods are fairly thin wrappers around the rust struct which manages mutability already.

Since all the existing tests passed (at least locally) that should be reasonable confidence this isn't totally flawed. I could add a test related to my repro just to show threads work now.

Are there any user-facing changes?

There shouldn't be.

Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

It doesn't look like there are any actual changes here? You're just switching to not use &mut?

@kylebarron
Copy link
Member

I think a better PR description might be "Make pyclasses frozen to use our own interior mutability"

@ntjohnson1 ntjohnson1 changed the title Allow Python Threading Around Session Context Make Session Context pyclass frozen so interior mutability is only managed by rust Sep 23, 2025
@ntjohnson1
Copy link
Contributor Author

It doesn't look like there are any actual changes here? You're just switching to not use &mut?

Correct. Basically marked the class as frozen to make sure no mut self accidentally creep in. When not mut self then python threads are enabled since we aren't trying to borrow mutable versions of the python wrapper twice. Then I believe the rust compiler verifies nothing deeper in the accesses actual needed a mutable version of the rust Session Context and are safe for concurrency (things are Arc etc all the way down).

I think a better PR description might be "Make pyclasses frozen to use our own interior mutability"

Updated the description. Can update to your suggestion verbatim if still unclear.

@kylebarron
Copy link
Member

kylebarron commented Sep 23, 2025

Looks great! I'd say all pyclasses should be frozen by default (and I think pyo3 intends to make that the default in the future?). I think in terms of title I was just confused because it doesn't directly relate to threading at all; it's just an indirect incompatibility through the pyo3 implementation

@ntjohnson1
Copy link
Contributor Author

I'd say all pyclasses should be frozen by default (and I think pyo3 intends to make that the default in the future?).

I can create an issue for follow up on this to move more classes in the repo to frozen. Don't think I'll have a chance to get to it immediately. Mostly just solved this because by the time I root caused the issue it was basically fixed.

Re: pyo3 frozen default that is my understanding as well based on the note under the code block

Copy link
Member

@timsaucer timsaucer left a comment

Choose a reason for hiding this comment

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

Thank you Nick! It's nice to see my plans to turn you into a rust developer coming together!

@timsaucer timsaucer merged commit f08d5b0 into apache:main Sep 24, 2025
16 checks passed
@timsaucer
Copy link
Member

Thank you @ntjohnson1 and @kylebarron

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.

Already Borrowed Error When using SessionContext from Multiple Threads

3 participants