8000 Fix crash in AsyncSpec when using TestState by younata · Pull Request #1261 · Quick/Quick · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix crash in AsyncSpec when using TestState #1261

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
Merged
merged 1 commit into from
Jan 17, 2024
Merged

Fix crash in AsyncSpec when using TestState #1261

merged 1 commit into from
Jan 17, 2024

Conversation

younata
Copy link
Member
@younata younata commented Jan 17, 2024

Fixes #1258

Sometimes, Using TestState in AsyncSpec would crash. I traced this down to a race condition where a new test could be started/running, and then we would unset AsyncSpec.current.

The diff here is fairly small, so let's step through it:

This is the actual test method that we create for XCTest, example.run is when we actually run the test code.

Was:

spec.example = example
Task {
    await example.run()
    completionHandler()
    AsyncSpec.current = nil
}

Now:

Task {
    spec.example = example
    await example.run()
    AsyncSpec.current = nil
    completionHandler()
}

While I'm fairly certain that moving spec.example = example into the Task isn't necessary, I also know that it doesn't hurt. For reference, as a side effect of setting spec.example, we also set AsyncSpec.current. See AsyncSpec.example.

The actual bug here is setting AsyncSpec.current = nil after we call completionHandler. completionHandler is where we tell XCTest that we've finished running the test and it can move on (because we're integrating with an Objective-C API, and lose some safety there). I can definitely see the not-uncommon case where XCTest would start running the next test after we call completionHandler but before we had executed the AsyncSpec.current = nil line. For example, the following control flow:

Thread 1 (running test 1): completionHandler()
Thread 2 (running test 2): spec.example = example
Thread 1: AsyncSpec.current = nil
Thread 2: await example.run()
...

It's useful to remember that, despite XCTest supporting async specs, XCTest does not support running tests concurrently in the same process (and for good reasons - way too easy to trip over any of the many singletons in an app). When you run tests concurrently, XCTest spins up multiple processes to run them.

Anyway, by moving the AsyncSpec.current = nil to before we call completionHandler(), we can be sure that unsetting AsyncSpec.current will happen before we run the next test.

@github-actions GitHub Actions
Copy link
1 Warning
⚠️ Need to add an unit test if you're modifying swift source

Generated by 🚫 Danger

@younata younata merged commit e84f6fe into main Jan 17, 2024
@younata younata deleted the fix_crash_in_asyncspec_when_using_teststate branch January 17, 2024 18:25
cgrindel-self-hosted-renovate bot referenced this pull request in cgrindel/rules_swift_package_manager Jan 18, 2024
@cgrindel-self-hosted-renovate
chore(deps): update dependency quick/quick to from: "7.3.1" (#860)
9589ca8 
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [Quick/Quick](https://togithub.com/Quick/Quick) | patch | `from:
"7.3.0"` -> `from: "7.3.1"` |

---

### Release Notes

<details>
<summary>Quick/Quick (Quick/Quick)</summary>

### [`v7.3.1`](https://togithub.com/Quick/Quick/releases/tag/v7.3.1)

[Compare
Source](https://togithub.com/Quick/Quick/compare/v7.3.0...v7.3.1)

### Highlights

- Fixes a crash in AsyncSpec caused by race condition when unsetting
AsyncSpec.current.

### Autogenerated Release Notes

#### What's Changed

- Bump danger from 9.3.1 to 9.3.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/Quick/Quick/pull/1243](https://togithub.com/Quick/Quick/pull/1243)
- Bump cocoapods from 1.12.1 to 1.13.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/Quick/Quick/pull/1244](https://togithub.com/Quick/Quick/pull/1244)
- Bump fkirc/skip-duplicate-actions from 5.3.0 to 5.3.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/Quick/Quick/pull/1245](https://togithub.com/Quick/Quick/pull/1245)
- Bump cocoapods from 1.13.0 to 1.14.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/Quick/Quick/pull/1247](https://togithub.com/Quick/Quick/pull/1247)
- Bump rake from 13.0.6 to 13.1.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/Quick/Quick/pull/1248](https://togithub.com/Quick/Quick/pull/1248)
- Consolidate Quick-macOS, Quick-iOS and Quick-tvOS into a single Quick
target by [@&#8203;younata](https://togithub.com/younata) in
[https://github.com/Quick/Quick/pull/1222](https://togithub.com/Quick/Quick/pull/1222)
- Bump danger from 9.3.2 to 9.4.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/Quick/Quick/pull/1254](https://togithub.com/Quick/Quick/pull/1254)
- Bump danger from 9.4.1 to 9.4.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/Quick/Quick/pull/1257](https://togithub.com/Quick/Quick/pull/1257)
- Bump cocoapods from 1.14.2 to 1.14.3 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/Quick/Quick/pull/1252](https://togithub.com/Quick/Quick/pull/1252)
- Build carthage artifacts as a github action. by
[@&#8203;younata](https://togithub.com/younata) in
[https://github.com/Quick/Quick/pull/1259](https://togithub.com/Quick/Quick/pull/1259)
- Add a privacy manifest by
[@&#8203;younata](https://togithub.com/younata) in
[https://github.com/Quick/Quick/pull/1260](https://togithub.com/Quick/Quick/pull/1260)
- Fix crash in AsyncSpec when using TestState by
[@&#8203;younata](https://togithub.com/younata) in
[https://github.com/Quick/Quick/pull/1261](https://togithub.com/Quick/Quick/pull/1261)

**Full Changelog**:
Quick/Quick@v7.3.0...v7.3.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDAuMCIsInVwZGF0ZWRJblZlciI6IjM2LjEwMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sometimes TestState crashes for AsyncSpec
1 participant
@younata
0