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

python: Revive ws-demo #190

Merged
merged 4 commits into from
Feb 6, 2025
Merged

python: Revive ws-demo #190

merged 4 commits into from
Feb 6, 2025

Conversation

gasmith
Copy link
Contributor

@gasmith gasmith commented Feb 6, 2025

Revive the python ws-demo, by creating a tokio runtime on demand.

Changes here:

  • Create a runtime on demand if one doesn't exist.
  • Hoist get_runtime_handle() out of websocket.rs to runtime.rs for possible future reuse.
  • Teach get_runtime_handle() to reuse the current runtime, if one exists, instead of always creating a new one.
  • Add a shutdown_runtime function, and invoke it from a python atexit hook.
  • Teach python to drop the GIL before calling into async code.

Those last two are critical for avoiding SIGABRTs and deadlocks during python program exit. Since we use the pyo3-log forwarder, our tokio threads have a tendency to grab the GIL for logging, which means that we need to drop the GIL before blocking on anything going on in tokio.

Fixes: FG-10384
Fixes: FG-9902

@gasmith gasmith self-assigned this Feb 6, 2025
Copy link

linear bot commented Feb 6, 2025

@gasmith gasmith force-pushed the gasmith/fg-10384-revive-py-ws-demo branch from dc4a846 to 66c93f2 Compare February 6, 2025 00:39
@gasmith gasmith marked this pull request as ready for review February 6, 2025 00:47
@gasmith gasmith requested review from bryfox and eloff February 6, 2025 00:47
Copy link
Contributor

@bryfox bryfox left a comment

Choose a reason for hiding this comment

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

👍 I believe the drop/abort handling resolves FG-9902 as well.

use an existing runtime when one exists

Worth a patch release of the crate?

@amacneil
Copy link
Contributor

amacneil commented Feb 6, 2025

tested and works for me

getting a protobuf deserialization error in FG, but that seems unrelated to the current PR

@gasmith
Copy link
Contributor Author

gasmith commented Feb 6, 2025

Figured it out.

Turns out the GIL is held when you enter a #[pyfunction] or #[pymethod]. Seems completely obvious in retrospect, but I wasn't thinking about it. We really need to drop that before we do a block_on(future). This is particularly important because of pyo3-log, which grabs the GIL to write logs.

By dropping the GIL at the appropriate times, we no longer need the Runtime::shutdown_timeout hack. We can just drop the Runtime, and everything gets cleaned up.

Copy link

linear bot commented Feb 6, 2025

@gasmith
Copy link
Contributor Author

gasmith commented Feb 6, 2025

👍 I believe the drop/abort handling resolves FG-9902 as well.

It certainly brings us in line with the rust implementation, so we'll mark it fixed for now. Thanks.

use an existing runtime when one exists

Worth a patch release of the crate?

I don't think so. It's OK to have multiple tokio runtimes. In fact, the isolation might not be such a bad thing. But it does have a cost, and I think our default should be to reuse an existing runtime when we have one.

@gasmith gasmith merged commit 48791a0 into main Feb 6, 2025
27 checks passed
@gasmith gasmith deleted the gasmith/fg-10384-revive-py-ws-demo branch February 6, 2025 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants