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 crash when accessing setValue concurrently (follow-up) #5

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

Conversation

hokuron
Copy link

@hokuron hokuron commented Dec 10, 2024

This is a follow-up PR to #2.

Closes #2

@hokuron
Copy link
Author

hokuron commented Dec 10, 2024

Does Package.swift need to be updated with these changes?
#2 (comment)

@hokuron hokuron force-pushed the support-concurrently-access branch from 55f631f to b02d093 Compare December 10, 2024 07:39
@elmetal
Copy link
Contributor

elmetal commented Dec 11, 2024

Does Package.swift need to be updated with these changes? #2 (comment)

I think it is unnecessary since it won't e needed if we upgrade the macOS version in GitHub Actions. What do you think? @ichiho-ojima

@ichiho-ojima
Copy link
Contributor

@elmetal @hokuron
As you pointed out, one solution is to upgrade the macOS version and build in Swift 6 language mode. Alternatively, I believe adding .enableExperimentalFeature("StrictConcurrency") to Package.swift will also resolve this issue.

var storage: [CacheKey : Data] = [:]
let storage: ThreadSafeDictionary<CacheKey, Data> = .init()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the crash issue cannot be resolved simply by wrapping storage with OSAllocatedUnfairLock.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'll try it.

Copy link
Author

Choose a reason for hiding this comment

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

@ichiho-ojima
OSAllocatedUnfairLock requires iOS 16+, tvOS 16+, watchOS 9+, and macOS 13+. Can we bump our minimum deployment targets to these versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I forgot to consider the version where OSAllocatedUnfairLock is available.
Personally, I think it's an option to make the supported version iOS 16+ for using OSAllocatedUnfairLock, but what do you think? @elmetal

Copy link
Contributor

Choose a reason for hiding this comment

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

@ichiho-ojima
I think that's a good approach, but we need to be mindful of versioning.

@hokuron
Copy link
Author

hokuron commented Dec 12, 2024

@ichiho-ojima @elmetal

As you pointed out, one solution is to upgrade the macOS version and build in Swift 6 language mode. Alternatively, I believe adding .enableExperimentalFeature("StrictConcurrency") to Package.swift will also resolve this issue.

Should we keep this PR focused on adding .enableExperimentalFeature("StrictConcurrency") and address the Swift 6 Language Mode migration in a separate PR?

@ichiho-ojima
Copy link
Contributor

@hokuron

Should we keep this PR focused on adding .enableExperimentalFeature("StrictConcurrency") and address the Swift 6 Language Mode migration in a separate PR?

If migrating to Swift 6 is very straightforward, I think it would be fine to handle it in this PR.
If it's challenging, I recommend splitting it into another PR.

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