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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removed forced optional type used in Atomic
#254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Atomic is most probably not used by end users so I'm not sure to put this under "Breaking Changes", I'd say we put it under "Changed" so we can release it in 2.2
CHANGELOG.md
Outdated
### 鈿狅笍 Breaking Changes | ||
- `Atomic.get()` no longer returns an optional type if the wrapped type itself is not optional. `Atomic.init(_:)` now always requires the initial value [#241](https://github.com/GetStream/stream-chat-swift/issues/241). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should merge this into release/3.0
since it's breaking and touches a lot of different parts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to merge it to master
. Let me redo it in a bit more compatible way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Finally non-optional!
Only the same question about CHANGELOG.
2cb72f3
to
3597392
Compare
DispatchQueue.random.async { | ||
// We need to capture the value explicitly. Without this the tests randomly crashes after | ||
// `atomicValue` is assigned to `nil`. | ||
DispatchQueue.random.async { [atomicValue] in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 minutes to identify the problem + 50 minutes figuring out how to fix it = [atomicValue]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate a write-up stating why explicit capture solved the crash!
馃殺
/// - Note: Even though the value guarded by `Atomic` is thread-safe, the `Atomic` class itself is not. Mutating the instance | ||
/// itself from multiple threads can cause a crash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
馃憤
@b-onc As far as I understand it, capturing it explicitly creates a strong reference to |
3597392
to
1ad03e5
Compare
In the PR:
value
inAtomic
is no longer treated asOptional
by defaultAtomic
stress tests random crash (hopefully) fixed 馃@buh @b-onc Please review and test this extremely carefully. I touched a lot of different parts of the app and could easily introduce some copy-paste/typo kind of bug 馃檹