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

Fix thread-local shutdown of RecordingStream on Mac and WIndows #3937

Open
emilk opened this issue Oct 20, 2023 · 2 comments
Open

Fix thread-local shutdown of RecordingStream on Mac and WIndows #3937

emilk opened this issue Oct 20, 2023 · 2 comments
Labels
🪳 bug Something isn't working enhancement New feature or request
Milestone

Comments

@emilk
Copy link
Member

emilk commented Oct 20, 2023

Calling RecordingStream::set_thread_local on Mac and WIndows used to cause a crash during shutdown (#2889). In #3929 we replaced this with an ugly, arbitrary sleep instead.

This sleep has two downsides:

  • It adds latency during shutdown to Mac
  • It may not be enough to flush the pipe

A better fix would do something like:

while !flushed() {
    thread.yield()
}

The problem is we cannot use any channels for this, so we would need to rely on atomic bools and similar.

@emilk emilk added enhancement New feature or request 👀 needs triage This issue needs to be triaged by the Rerun team labels Oct 20, 2023
emilk added a commit that referenced this issue Oct 20, 2023
)

### What
* Closes #3558
* Closes #2889
* Opens #3937

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3929) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/3929)
- [Docs
preview](https://rerun.io/preview/2b1469294de742102f69f16f6ecb8dc0e3e3a0c5/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/2b1469294de742102f69f16f6ecb8dc0e3e3a0c5/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)

---------

Co-authored-by: Andreas Reich <[email protected]>
@emilk emilk added 🪳 bug Something isn't working and removed 👀 needs triage This issue needs to be triaged by the Rerun team labels Oct 23, 2023
@Wumpf
Copy link
Member

Wumpf commented Oct 27, 2023

We're seeing the same issue on Windows as well.

@emilk
Copy link
Member Author

emilk commented Oct 29, 2023

@emilk emilk changed the title Fix thread-local shutdown of RecordingStream on Mac Fix thread-local shutdown of RecordingStream on Mac and WIndows Oct 30, 2023
@emilk emilk added this to the Triage milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants