Skip to content

Close file handle in cancellation handler to avoid race #174

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KaiOelfke
Copy link

This should fix #172 and implements the file handle closing as recommended in the Apple documentation.

@mbrandonw
Copy link
Member

Hi @KaiOelfke, thanks so much for taking this on. Can you think of any way we can get a test in place that would have crashed without the change? Would be nice to have a record of why we made this change for our future selves.

@KaiOelfke
Copy link
Author

KaiOelfke commented Jul 18, 2025

@mbrandonw

In the issue I have added a comment with an unit test, but the problem is that the test is slow and non deterministic. I tried to improve the test to more reliably run into this race condition, but I didn't manage that so far. Feel free to look at the issue and the documentation by Apple for the cancellation handler to see, if you can improve the test.

I didn't commit this test as it's in my opinion too slow and flaky in the current shape.

@KaiOelfke
Copy link
Author

@mbrandonw FYI, I've released an app update a while ago with a fork with this PR. It eliminates my biggest app crash group in the Xcode organizer. While a better test would be desirable at least there's one here in the ticket that reproduces the problem slowly and as a reference one could also mention Apple's documentation.

I'm sure there will be other apps out there crashing with this bug. Just maybe less frequently depending on how they use file storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileStorage crash due to incorrect closure of file descriptor causing a race condition
2 participants