Conversation
|
🚨 End to end tests failed. Please check the failed workflow run. |
acb-mv
left a comment
There was a problem hiding this comment.
@acb-mv reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved.
ios/MullvadVPNTests/MullvadTypes/FileCacheTests.swift line 81 at r1 (raw file):
// MARK: - Thundering herd
Good tests
Yes, tests that fail 😢 . |
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 2 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pinkisemils).
ios/MullvadVPNTests/MullvadTypes/FileCacheTests.swift line 227 at r1 (raw file):
} // Both caches should be able to read a consistent value.
Isn't it more interesting to see if they contain the same value?
ios/MullvadTypes/FileCache.swift line 70 at r1 (raw file):
let content = try JSONDecoder().decode(Content.self, from: Data(contentsOf: url)) cacheQueue.sync { cachedContent = content }
Why do we write to memory here. It should already be handled in write below.
ios/MullvadTypes/FileCache.swift line 105 at r1 (raw file):
cacheQueue.sync { cachedContent = content } } catch {
A log would be good here. Otherwise we can achieve the same thing with try? JSONDecoder[...] and get rid of the catch.
5e9797e to
483e487
Compare
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 3 comments.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @acb-mv, @pinkisemils, and @rablador).
ios/MullvadTypes/FileCache.swift line 70 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Why do we write to memory here. It should already be handled in
writebelow.
I guess it initiate the cachedContent if no cache is available.
ios/MullvadTypes/FileCache.swift line 105 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
A log would be good here. Otherwise we can achieve the same thing with
try? JSONDecoder[...]and get rid of the catch.
+1
ios/MullvadVPNTests/MullvadTypes/FileCacheTests.swift line 45 at r2 (raw file):
} // MARK: - Cache behavior
typo: behaviour
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pinkisemils).
483e487 to
73f25c3
Compare
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rablador).
ios/MullvadTypes/FileCache.swift line 70 at r1 (raw file):
Previously, mojganii wrote…
I guess it initiate the
cachedContentif no cache is available.
If the cache is empty, we must read from the file itself.
ios/MullvadTypes/FileCache.swift line 105 at r1 (raw file):
Previously, mojganii wrote…
+1
We can't have a logger here since we don't want to depend on MullvadTypes :(
Fixed to not use a do block here.
ios/MullvadVPNTests/MullvadTypes/FileCacheTests.swift line 227 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Isn't it more interesting to see if they contain the same value?
Done.
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 2 files and all commit messages, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved.
73f25c3 to
a917164
Compare
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved.
rablador
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved.
We have deadlocks in the FileCache. Those are all the more pressing on iOS17, where the File Coordinator is broken (it notifies all presenters, even the one that is passed to the Coordinator upon initialization). These changes remove the lock, and instead guard the cache on a queue. I've also added some tests that fail to pass on main but do pass with the introduced changes.
This change is