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.

Sign up for GitHub

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

Jump to bottom

Fix crash when using RecordingStream::set_thread_local on macOS #3929

Merged
merged 8 commits into from Oct 20, 2023
Merged

Fix crash when using RecordingStream::set_thread_local on macOS #3929

merged 8 commits into from Oct 20, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Oct 19, 2023 •
edited by github-actions bot

What

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 demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@emilk emilk added 💣 crash crash, deadlock/freeze, do-no-start include in changelog 🦀 Rust API Rust logging API 🌊 C++ API C or C++ logging API labels Oct 19, 2023
@Wumpf Wumpf self-requested a review October 19, 2023 15:23
Wumpf
Wumpf approved these changes Oct 19, 2023
View reviewed changes
Copy link
Member

@Wumpf Wumpf left a comment •
edited

Choose a reason for hiding this comment

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

shudder

great detective work though :notbad:

crates/re_sdk/src/global.rs Outdated Show resolved Hide resolved
crates/re_sdk/src/global.rs
re_log::warn!("Using thread-local RecordingStream on macOS can result in data loss because of https://github.com/rerun-io/rerun/issues/2889");

// Give the batchet and sink threads a chance to process the data.
std::thread::sleep(std::time::Duration::from_millis(500));
Copy link
Member

Choose a reason for hiding this comment

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

Tbh not sure if we already talked about this, but there's no way easy way right now to know whether the threads are still busy, right? Would be nice to be able to tell them to join instead 🤔
Adding 500ms to our shutdown time is pretty awful and error prone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's not great, but we would need to check if all the queues are empty and the threads non-busy all the way down, and it is not trivial

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll open a new issue (and close the old one) as a future TODO

Copy link
Member Author

Choose a reason for hiding this comment

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

#3937

@emilk @Wumpf
typo
de19a48 
Co-authored-by: Andreas Reich <andreas@rerun.io>
@emilk emilk mentioned this pull request Oct 20, 2023
Open
@emilk emilk merged commit a8bb2f1 into main Oct 20, 2023
32 checks passed
@emilk emilk deleted the emilk/workaround-thread-crash-on-mac branch October 20, 2023 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wumpf Wumpf Wumpf approved these changes

Assignees
No one assigned
Labels
🌊 C++ API C or C++ logging API 💣 crash crash, deadlock/freeze, do-no-start include in changelog 🦀 Rust API Rust logging API
Projects
None yet
Milestone
No milestone
2 participants
@emilk @Wumpf