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

Multithreaded Environment Support #110

Closed
dhan2325 opened this issue May 10, 2024 · 8 comments · Fixed by #137
Closed

Multithreaded Environment Support #110

dhan2325 opened this issue May 10, 2024 · 8 comments · Fixed by #137
Labels
enhancement New feature or request

Comments

@dhan2325
Copy link

Problem

My team and I were hoping to use py-crdt in a server-side context with multiple workers. We ran into the issue of workers switching context and the rust binary panicking when resources were dropped by the original threads, complaining that the structs are "unsendable".

I skimmed through the source code and found the Rust definitions for the pycrdt structs that were causing these issues, and noticed that they were simple wrappers around yrs structs which do implement Send and Sync, but that the wrappers around them are explicitly marked as unsendable, which I thought might be related.

I did also see in related threads in other repositories that an async API system was being considered, and wanted to ask if there were any updates on that front, and if there was some other considerations I am unaware of that required structs to be unsendable? And if there was any proposed solution to this issue that is in the works?

Any information anyone could provide would be very helpful, thanks!

@dhan2325 dhan2325 added the enhancement New feature or request label May 10, 2024
Copy link

welcome bot commented May 10, 2024

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@davidbrochart
Copy link
Collaborator

There is a comment pointing to what should be done to allow multithreading, but I haven't gone any further.
I'm still interested though, and it would be great if you wanted to open a PR.

@davidbrochart
Copy link
Collaborator

I opened #114.

@dhan2325
Copy link
Author

Thank you for the prompt response! I took a look at the PR, and tried cloning + building but taking a closer look it looks as though some of the underlying yrs structs don't implement send and at some point we may need some Mutexing to make this thread-safe?

Performance issues aside, would wrapping each of the underlying yrs structs in the Python interface layer in an Arc<Mutex<T>> solve the issue? If so, is this something that would be helpful to implement and then provide some library interface that allows user to decide if they'd like to use a mutexed version of the library?

@davidbrochart
Copy link
Collaborator

Good questions, I honestly don't know. Should we start by just having it work, using mutexes?

@dhan2325
Copy link
Author

dhan2325 commented May 14, 2024

I think a Mutex implementation would be very nice to have, so long as it's kept as an optional configuration - applications that are single-threaded shouldn't incur the overhead of locking, etc.

@davidbrochart
Copy link
Collaborator

Would this configuration consist of having two separate builds, one with mutexes and one without, and a runtime parameter for choosing one?

@davidbrochart
Copy link
Collaborator

Note that even when not sending objects across threads, just creating an object in another thread is problematic since it can be garbage collected from a different thread, which will panic and lead to memory leaks. We can see that in pytest for instance, see here.
So it might be safer to always use mutexes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants