Skip to content

Fix python SDK's shutdown unsafely dropping cross-FFI resources#8038

Merged
teh-cmc merged 1 commit intomainfrom
cmc/fix_python_shutdown
Nov 7, 2024
Merged

Fix python SDK's shutdown unsafely dropping cross-FFI resources#8038
teh-cmc merged 1 commit intomainfrom
cmc/fix_python_shutdown

Conversation

@teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Nov 7, 2024 •
edited by github-actions bot
Loading

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
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

@teh-cmc teh-cmc added 🪳 bug Something isn't working sdk-python Python logging API 💣 crash crash, deadlock/freeze, do-no-start include in changelog 🚢 CI labels Nov 7, 2024
@teh-cmc
Copy link
Member Author

teh-cmc commented Nov 7, 2024

This one should turn green: https://github.com/rerun-io/rerun/actions/runs/11724715409?pr=7968

@teh-cmc
Copy link
Member Author

teh-cmc commented Nov 7, 2024

@rerun-bot full-check

@github-actions
Copy link

github-actions bot commented Nov 7, 2024

Started a full build: https://github.com/rerun-io/rerun/actions/runs/11724820546

zehiko
zehiko reviewed Nov 7, 2024
View reviewed changes
rerun_py/src/python_bridge.rs
for (_, recording) in all_recordings().drain() {
// NOTE: Do **NOT** try and drain() `all_recordings` here.
//
// Doing so would drop the last remaining reference to these recordings, and therefore
Copy link
Member

@zehiko zehiko Nov 7, 2024

Choose a reason for hiding this comment

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

thanks for nicely clarifying, this is indeed nuanced

@teh-cmc teh-cmc merged commit b68176d into main Nov 7, 2024
@teh-cmc teh-cmc deleted the cmc/fix_python_shutdown branch November 7, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zehiko zehiko zehiko approved these changes

Assignees

No one assigned

Labels

🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start include in changelog sdk-python Python logging API 🚢 CI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CI failing: "terminate called without an active exception"

2 participants

@teh-cmc @zehiko