Skip to content

Close manifest cache DB before calling completion handlers #5881

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

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

neonichu
Copy link
Contributor

@neonichu neonichu commented Nov 4, 2022

Previously, we were relying on defer to close the database, but that would mean we're closing it when exiting the scope which is typically after we are calling completion handlers. It seems as if we should instead call them before.

Related to rdar://101956252

@neonichu neonichu self-assigned this Nov 4, 2022
@neonichu
Copy link
Contributor Author

neonichu commented Nov 4, 2022

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Nov 4, 2022

Hm, looks like this isn't working well: Basics/SQLiteBackedCache.swift:66: Fatal error: db should be closed

@neonichu
Copy link
Contributor Author

neonichu commented Nov 4, 2022

I think we should probably make a bigger change here actually, it doesn't seem right that we're calling the completion handler before closing the DB.

Previously, we were relying on `defer` to close the database, but that would mean we're closing it when exiting the scope which is typically *after* we are calling completion handlers. It seems as if we should instead call them before.

Related to rdar://101956252
@neonichu neonichu force-pushed the fix-defer-in-manifest-caching branch from eda4114 to e5124cc Compare November 4, 2022 19:41
@neonichu neonichu changed the title Fix misplaced defer in manifest caching Close manifest cache DB before calling completion handlers Nov 4, 2022
@neonichu
Copy link
Contributor Author

neonichu commented Nov 4, 2022

@swift-ci please smoke test

@neonichu neonichu merged commit 1c53ef5 into main Nov 7, 2022
@neonichu neonichu deleted the fix-defer-in-manifest-caching branch November 7, 2022 22:26
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.

3 participants