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 SDK: introduce deferred garbage collection queue #4583

Merged
merged 10 commits into from
Jan 2, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Dec 19, 2023

This introduces a new deferred system to clear Arrow garbage that was originally allocated in Python land.

This should fix all deadlocks/segfaults/aborts past, present and future... or not 🥲

NOTE: This lives in parallel to the already existing ALL_RECORDINGS thingy, which is still needed to avoid killing and joining threads at a bad time.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added 🪳 bug Something isn't working 🐍 Python API Python logging API do-not-merge Do not merge this PR include in changelog labels Dec 19, 2023
@teh-cmc teh-cmc force-pushed the cmc/recstream_buffered_py_crash branch from c1a2bc8 to 1950030 Compare December 22, 2023 15:26
@teh-cmc teh-cmc changed the title Fix python aborts/deadlocks/other-fun-things when dropping recordings Python SDK: introduce deferred garbage collection queue Dec 22, 2023
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Dec 22, 2023
@teh-cmc teh-cmc force-pushed the cmc/recstream_buffered_py_crash branch 4 times, most recently from 3ae1791 to 74e7d45 Compare December 22, 2023 15:39
@teh-cmc teh-cmc force-pushed the cmc/recstream_buffered_py_crash branch from 74e7d45 to eaf8383 Compare December 22, 2023 16:02
@teh-cmc teh-cmc marked this pull request as ready for review December 22, 2023 16:11
@jleibs jleibs self-requested a review December 22, 2023 17:16
@jleibs
Copy link
Member

jleibs commented Dec 22, 2023

This feels like it might be a better implementation of the attempted workaround that existed in buffered_client:

I suspect this means that code can go away and stuff can be safely dropped after packet sending instead of shuffling off to yet-another thread.

Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

Makes sense.

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 2, 2024

This feels like it might be a better implementation of the attempted workaround that existed in buffered_client:

* https://github.com/rerun-io/rerun/blob/jleibs/proper_clear_containers/crates/re_sdk_comms/src/buffered_client.rs#L185-L204

I suspect this means that code can go away and stuff can be safely dropped after packet sending instead of shuffling off to yet-another thread.

Nice catch, it's gone 🥳

@teh-cmc teh-cmc merged commit 61c23af into main Jan 2, 2024
40 checks passed
@teh-cmc teh-cmc deleted the cmc/recstream_buffered_py_crash branch January 2, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working include in changelog 🐍 Python API Python logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants