Skip to content

fix race condition when trie generation thread may be invoked after storage has been closed #3505

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
May 19, 2021

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented May 19, 2021

motivation: with trie genration now running on lower priority, a race was exposed where the operation may start after the storage already was closed

changes: guard on state before trying to execute a statement on the connection since it may be terminated

@tomerd
Copy link
Contributor Author

tomerd commented May 19, 2021

@swift-ci please smoke test

Copy link
Contributor

@yim-lee yim-lee left a comment

Choose a reason for hiding this comment

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

👍 Thanks @tomerd. I verified locally that with this patch I no longer see the error:

PackageCollections/SQLitePackageCollectionsStorage.swift:74: Fatal error: db should be closed

in tests.

…torage has been closed

motivation: with trie genration now running on lower priority, a race was exposed where the operation may start after the storage already was closed

changes:
* in trie generation, guard on state before trying to execute a statement on the connection since it may be terminated
* in SQLite storage assert state does not got back frmo disconnected -> connected to help triage such issues
* adjust test to new assertion
@tomerd tomerd force-pushed the fix/trie-generation-race branch from 191c3ee to 5b2b7d6 Compare May 19, 2021 01:17
@tomerd
Copy link
Contributor Author

tomerd commented May 19, 2021

@swift-ci please smoke test

@tomerd tomerd merged commit b620227 into swiftlang:main May 19, 2021
tomerd added a commit to tomerd/swift-package-manager that referenced this pull request May 19, 2021
…torage has been closed (swiftlang#3505)

motivation: with trie genration now running on lower priority, a race was exposed where the operation may start after the storage already was closed

changes:
* in trie generation, guard on state before trying to execute a statement on the connection since it may be terminated
* in SQLite storage assert state does not got back frmo disconnected -> connected to help triage such issues
* adjust test to new assertion
yim-lee added a commit to yim-lee/swift-package-manager that referenced this pull request May 26, 2021
…e closed

Motivation:
With swiftlang#3505, the code does `preconditionFailure` when trying to execute DB statement after connection is disconnected. In `populateTargetTrie` we check connection state before calling `executeStatement`, but it's still possible for storage to close _after_ the check but _before_ `executeStatement`. This might have caused rdar://78513692.

Modifications:
- Throw error instead of `preconditionFailure`
- Unrelated: `GitHubPackageMetadataProvider.configuration` doesn't need to be var
yim-lee added a commit that referenced this pull request May 26, 2021
…e closed (#3512)

Motivation:
With #3505, the code does `preconditionFailure` when trying to execute DB statement after connection is disconnected. In `populateTargetTrie` we check connection state before calling `executeStatement`, but it's still possible for storage to close _after_ the check but _before_ `executeStatement`. This might have caused rdar://78513692.

Modifications:
- Throw error instead of `preconditionFailure`
- Unrelated: `GitHubPackageMetadataProvider.configuration` doesn't need to be var
yim-lee added a commit to yim-lee/swift-package-manager that referenced this pull request May 26, 2021
…e closed

Motivation:
With swiftlang#3505, the code does `preconditionFailure` when trying to execute DB statement after connection is disconnected. In `populateTargetTrie` we check connection state before calling `executeStatement`, but it's still possible for storage to close _after_ the check but _before_ `executeStatement`. This might have caused rdar://78513692.

Modifications:
- Throw error instead of `preconditionFailure`
- Unrelated: `GitHubPackageMetadataProvider.configuration` doesn't need to be var
yim-lee added a commit that referenced this pull request May 27, 2021
…e closed (#3513)

Motivation:
With #3505, the code does `preconditionFailure` when trying to execute DB statement after connection is disconnected. In `populateTargetTrie` we check connection state before calling `executeStatement`, but it's still possible for storage to close _after_ the check but _before_ `executeStatement`. This might have caused rdar://78513692.

Modifications:
- Throw error instead of `preconditionFailure`
- Unrelated: `GitHubPackageMetadataProvider.configuration` doesn't need to be var
bitjammer pushed a commit to bitjammer/swift-package-manager that referenced this pull request Jul 23, 2021
…torage has been closed (swiftlang#3505)

motivation: with trie genration now running on lower priority, a race was exposed where the operation may start after the storage already was closed

changes:
* in trie generation, guard on state before trying to execute a statement on the connection since it may be terminated
* in SQLite storage assert state does not got back frmo disconnected -> connected to help triage such issues
* adjust test to new assertion
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.

2 participants