[SR-10374] URLCache: init method and first time sqlite database setup implemented #2000

karthikkeyan · 2019-03-13T20:41:50Z

init method implemented
URLCache.shared singleton object created with 4MB of memory space and 20MB of disk space
Directory and database file Cache.db file created under the local directory
Sqlite Tables and Indices created in the database
Unit tests added for URLCache to verify directory, file, tables, and indices

Bug Link: https://bugs.swift.org/browse/SR-10374

pushkarnk · 2019-04-16T05:14:52Z

@karthikkeyan Apologies for the very late response on this PR. I think implementations of URLCache and URLCredentialStorage have been long pending and this looks like a great start! However, I've a few questions about this pull request:

Is SQLite available on Linux and Windows? I guess not.
Have we considered other storage alternatives?
I came across this old discussion on URLCache . You might find it relevant.

karthikkeyan · 2019-04-16T18:59:32Z

@karthikkeyan Apologies for the very late response on this PR. I think implementations of URLCache and URLCredentialStorage have been long pending and this looks like a great start! However, I've a few questions about this pull request:

Is SQLite available on Linux and Windows? I guess not.

Have we considered other storage alternatives?
I came across this old discussion on URLCache . You might find it relevant.

I have failed to give a fair bit of a thought process about the Linux platform. Thanks for reminding of it, I will follow the discussion and see how best we can move this PR forward.

karthikkeyan · 2019-04-26T21:05:06Z

@pushkarnk I have reimplemented store and retrieve logic using NSKeyedArchiver and NSKeyedUnarchiver. This implementation is not perfect but I think it would be a good start.

spevans · 2019-04-28T07:47:46Z

@swift-ci test

drodriguez

Full disclosure: I didn’t know about this PR, and I started working on my own URLCache (that's the reason for #2245). I have some parts that are present in this version (with some differences), but I focused in the memory cache first. I have ideas for the disk cache, but that's more complicated. I can also have a lot more feedback about things I know that will need to be handle (for example skipping some protocols from being cached). Are you still planning in working on this?

drodriguez · 2019-05-21T23:09:28Z

Foundation/URLCache.swift

+
+            sharedSyncQ.sync {
+                // Remove previous data before resetting new URLCache instance
+                URLCache.sharedCache?.persistence?.deleteAllCaches()


Does the original implementation do this? Feels weird removing “user” data (even if it is a cache), just by assigning another value.

It does not.

drodriguez · 2019-05-21T23:11:16Z

Foundation/URLCache.swift

+
+    let path: String
+
+    var cacheSizeInDesk: Int {


Typo: cacheSizeInDisk

drodriguez · 2019-05-21T23:15:02Z

Foundation/URLCache.swift

+                let attributes = try FileManager.default.attributesOfItem(atPath: path.appending(fileName))
+                total += (attributes[.size] as? Int) ?? 0
+            }
+            return total


Is there plan to keep this value updated as cached responses are added and evicted? Feels like with a big cache and a slow disk, each of these calls might take a while.

Let me add a FIXME comment, I will create a separate PR for optimizing this. My main focus in this PR is to create an initial setup and kick start the implementation. Hope that's ok.

drodriguez · 2019-05-21T23:16:51Z

Foundation/URLCache.swift

+    func setupCacheDirectory() {
+        do {
+            if FileManager.default.fileExists(atPath: path) {
+                try FileManager.default.removeItem(atPath: path)


Wouldn’t this delete the previous cache contents?

drodriguez · 2019-05-21T23:21:27Z

Foundation/URLCache.swift

+    }
+
+    private func urlForFileIdentifier(_ identifier: String) -> URL {
+        return URL(fileURLWithPath: path + "/" + identifier)


It would be good to use URL(fileURLWithPath: path, isDirectory: true).appendingPathComponent(identifier) to handle platforms with other directory separators.

drodriguez · 2019-05-21T23:24:51Z

Foundation/URLCache.swift

+        }
+
+        let method = httpMethod ?? "GET"
+        return urlString + method


Since this is used as part of the path, shoulnd’t the output be escaped or hashed somehow?

karthikkeyan · 2019-05-22T22:00:27Z

Full disclosure: I didn’t know about this PR, and I started working on my own URLCache (that's the reason for #2245). I have some parts that are present in this version (with some differences), but I focused in the memory cache first. I have ideas for the disk cache, but that's more complicated. I can also have a lot more feedback about things I know that will need to be handle (for example skipping some protocols from being cached). Are you still planning in working on this?

@drodriguez Sorry for the delay response, yes I will be working on this PR. I was just waiting for someone to review this. Please give me your suggestions.

drodriguez

I will see if I submit my memory cache side, since the two are complementary (but we overlap in some points in which we already differ).

drodriguez · 2019-05-24T00:27:50Z

Foundation/URLCache.swift

+                } else {
+                    let fourMegaByte = 4 * 1024 * 1024
+                    let twentyMegaByte = 20 * 1024 * 1024
+                    let bundleIdentifier = Bundle.main.bundleIdentifier ?? UUID().uuidString


That UUID at the end is a little bit worrying. The app will fill caches, and will never reuse the cached results, filling the disk of garbage. I can think at least of ProcessInfo.processInfo.processName as a good fallback, but it is not unique (but the bundle identifier doesn’t have to be either). Maybe if no bundle identifier is found the best thing is not to have a disk cache at all.

Can this point to the temporary directory with a fixed name? (A sanitized process name does sound good to me!)

drodriguez · 2019-05-24T00:31:12Z

Foundation/URLCache.swift

+                    let fourMegaByte = 4 * 1024 * 1024
+                    let twentyMegaByte = 20 * 1024 * 1024
+                    let bundleIdentifier = Bundle.main.bundleIdentifier ?? UUID().uuidString
+                    let cacheDirectoryPath = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first?.path ?? "\(NSHomeDirectory())/Library/Caches"


Same idea here. If we don’t find a good URL for .cachesDirectory maybe it is better to not have disk cache.

The fallback has the problem of being very Darwin specific. For example NSHomeDirectory() in Android is the root directory, where one cannot write at all. Also, it might want to use appendingPathComponent(…).appendingPathComponent(…) to be nicer to Windows.

(There's another case below where the POSIX path separator is used instead of a higher level function).

The home directory should be in a persistent writable path everywhere, much as possible. I know that Android is really weird about this, but that looks like incorrect mapping to me.

Yeah, please do not fall back like this. If FileManager does not return a caches URL, consider not caching at all.

Sure, I will remove the fallback if FileManager for cachesDirectory returned nill.

drodriguez · 2019-05-24T00:35:33Z

Foundation/URLCache.swift

+        self.memoryCapacity = memoryCapacity
+        self.diskCapacity = diskCapacity
+
+        if let _path = path {


You might also want to check that diskCapacity is more than zero.

Darwin indeed does accept 0 arguments. My assumption is that memory capacity 0 means that as much as possible no data is kept in memory, and disk capacity 0 that no data is written to disk.

drodriguez · 2019-05-24T00:54:04Z

Foundation/URLCache.swift

+
+fileprivate extension URLRequest {
+
+    var cacheFileIdentifier: String {


I have a slightly different design here (I use a URLCacheKey type, instead of String), but one important thing is that generating a key from the URLRequest depends on some details from the request beside the URL itself. So my equivalent method returns an optional.

The first thing is the protocol. Protocols like data: or file: should not be cached for obvious reasons. I also think unknown protocols should not be cached either.

Also, for known protocols like http: or https: (and maybe for ftp:, but I don’t know that protocol that well), we should do a couple more things. For example:

I don’t think you can do a HTTP request without a method, so that's a return nil in the case the protocol is HTTP.

I think some headers should be taken into account. The spec refers to User-Agent, but others like Accept-Language and Range might be interesting.

I think the hash of the body contents should also be used.

I also think one should not try to cache request with body streams, because that will consume it, which is not good.

Finally I would hash everything and return an hexadecimal string, to have a valid file name.

millenomi · 2019-05-24T21:02:40Z

Foundation/URLCache.swift

+                } else {
+                    let fourMegaByte = 4 * 1024 * 1024
+                    let twentyMegaByte = 20 * 1024 * 1024
+                    let bundleIdentifier = Bundle.main.bundleIdentifier ?? UUID().uuidString


Can this point to the temporary directory with a fixed name? (A sanitized process name does sound good to me!)

millenomi · 2019-05-24T21:03:30Z

Foundation/URLCache.swift

+                    let fourMegaByte = 4 * 1024 * 1024
+                    let twentyMegaByte = 20 * 1024 * 1024
+                    let bundleIdentifier = Bundle.main.bundleIdentifier ?? UUID().uuidString
+                    let cacheDirectoryPath = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first?.path ?? "\(NSHomeDirectory())/Library/Caches"


Yeah, please do not fall back like this. If FileManager does not return a caches URL, consider not caching at all.

millenomi · 2019-05-24T21:03:53Z

Foundation/URLCache.swift

+
+            sharedSyncQ.sync {
+                // Remove previous data before resetting new URLCache instance
+                URLCache.sharedCache?.persistence?.deleteAllCaches()


It does not.

millenomi · 2019-05-24T21:07:16Z

Foundation/URLCache.swift

+        self.memoryCapacity = memoryCapacity
+        self.diskCapacity = diskCapacity
+
+        if let _path = path {


Darwin indeed does accept 0 arguments. My assumption is that memory capacity 0 means that as much as possible no data is kept in memory, and disk capacity 0 that no data is written to disk.

millenomi · 2019-05-24T21:08:02Z

Foundation/URLCache.swift

+    func setupCacheDirectory() {
+        do {
+            if FileManager.default.fileExists(atPath: path) {
+                try FileManager.default.removeItem(atPath: path)


millenomi · 2019-05-24T21:08:32Z

Foundation/URLCache.swift

+        }
+
+        let method = httpMethod ?? "GET"
+        return urlString + method


millenomi · 2019-05-24T21:09:04Z

I want to shepherd this patch into the repository. Do you need help with changes here?

karthikkeyan · 2019-05-24T21:48:25Z

I want to shepherd this patch into the repository. Do you need help with changes here?

@millenomi I am addressing all the comments added by you and Danial. I will let you know if I need any help. Thanks Lily.

karthikkeyan · 2019-05-25T23:41:58Z

@millenomi @drodriguez I have addressed all the comments. I still have some doubts about the implementation of the property cacheFileIdentifier, please let me know if there are any changes needed.

millenomi · 2019-05-28T17:41:47Z

@swift-ci please test

millenomi · 2019-05-28T17:46:46Z

Foundation/URLCache.swift

+            return nil
+        }
+
+        var hashString = "\(scheme)_\(method)_\(urlString)"


The collision space of the default hash is smaller than you probably think. Can we at least check when we load a request that this is actually the request we were looking for?

millenomi · 2019-05-28T17:46:55Z

Foundation/URLCache.swift

+
+        let url = urlForFileIdentifier(fileIdentifier)
+        guard let data = try? Data(contentsOf: url),
+            let response = NSKeyedUnarchiver.unarchiveObject(with: data) as? CachedURLResponse else {


Please use secure coding here.

To use secure coding, just invoke archivedData(withRootObject:requiringSecureCoding:) and unarchivedObject(of:from:) instead. You should've gotten deprecation warnings for these calls.

millenomi · 2019-05-28T17:47:16Z

Foundation/URLCache.swift

+    }
+
+    func saveCachedResponse(_ response: CachedURLResponse, for request: URLRequest) {
+        let archivedData = NSKeyedArchiver.archivedData(withRootObject: response)


This is slightly less urgent, but I'd like you to use secure coding wherever you use coding.

millenomi · 2019-05-28T17:48:34Z

Foundation/URLCache.swift

+        do {
+            try FileManager.default.createDirectory(atPath: path, withIntermediateDirectories: true)
+        } catch {
+            fatalError("Unable to create directories for URLCache: \(error.localizedDescription)")


Do not introduce fatal errors for things the user may not be able to control. As I asked elsewhere: consider not loading if we do not have a suitable directory.

millenomi · 2019-05-30T20:00:05Z

I'm going to commandeer this patch; I may drop the SQLite dependency, though.

karthikkeyan · 2019-05-30T20:43:00Z

I'm going to commandeer this patch; I may drop the SQLite dependency, though.

@millenomi I have pushed one more commit addressing the comments related to secure archiving. Also, I have updated the hashing mechanism for generating cacheFileIdentifier.

The one thing I am struggling though is to write a unit test for the internal property cacheFileIdentifier

* init method implemented * URLCache.shared singleton object created with 4MB of memory space and 20MB of disk space * Directory and database file Cache.db file created under local directory * Sqlite Tables and Indices created in database * Unit tests added for URLCache to verify directory, file, tables and indices

…it test

…d NSKeyedUnarchiver

…PI guidelines.

…eIdentifier hash mechanism updated.

karthikkeyan · 2020-04-04T22:44:25Z

Closing this PR as URLCache is implemented in this PR #2401

karthikkeyan force-pushed the karthik/urlcache-database-setup branch from 2ccc7c0 to 7793c62 Compare March 14, 2019 21:35

karthikkeyan changed the title ~~URLCache: init method and first time sqlite database setup implemented~~ [SR-10374] URLCache: init method and first time sqlite database setup implemented Apr 9, 2019

karthikkeyan force-pushed the karthik/urlcache-database-setup branch from 7793c62 to be57ae3 Compare April 9, 2019 21:49

karthikkeyan force-pushed the karthik/urlcache-database-setup branch from be57ae3 to ca9d675 Compare April 26, 2019 21:00

karthikkeyan force-pushed the karthik/urlcache-database-setup branch 2 times, most recently from 8875d8f to 0f21c7c Compare April 26, 2019 21:07

drodriguez reviewed May 21, 2019

View reviewed changes

drodriguez reviewed May 24, 2019

View reviewed changes

millenomi suggested changes May 24, 2019

View reviewed changes

karthikkeyan force-pushed the karthik/urlcache-database-setup branch from e1aa5c5 to cd746f3 Compare May 25, 2019 23:28

millenomi suggested changes May 28, 2019

View reviewed changes

drodriguez mentioned this pull request May 30, 2019

URLCache sketches. Mainly memory cache. #2312

Closed

karthikkeyan added 6 commits June 1, 2019 10:16

Database connection closed at the end of test_cacheDatabaseIndices un…

54b2676

…it test

URLCache store and fetch methods implemented using NSKeyedArchiver an…

02436cf

…d NSKeyedUnarchiver

Underscore added as prefix for privare type CachePersistence as per A…

5de096c

…PI guidelines.

Old cache directory keep in disk when new URLCache object assigned

ec38806

Secure archive and unarchive used for cache url response and cacheFil…

b5dafd2

…eIdentifier hash mechanism updated.

karthikkeyan force-pushed the karthik/urlcache-database-setup branch from d8e1009 to b5dafd2 Compare June 1, 2019 04:47

karthikkeyan closed this Apr 4, 2020


		fileprivate extension URLRequest {

		var cacheFileIdentifier: String {

[SR-10374] URLCache: init method and first time sqlite database setup implemented #2000

[SR-10374] URLCache: init method and first time sqlite database setup implemented #2000

Conversation

karthikkeyan commented Mar 13, 2019 • edited Loading

pushkarnk commented Apr 16, 2019

karthikkeyan commented Apr 16, 2019

karthikkeyan commented Apr 26, 2019

spevans commented Apr 28, 2019

drodriguez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikkeyan commented May 22, 2019

drodriguez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millenomi commented May 24, 2019

karthikkeyan commented May 24, 2019

karthikkeyan commented May 25, 2019 • edited Loading

millenomi commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millenomi May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millenomi commented May 30, 2019

karthikkeyan commented May 30, 2019

karthikkeyan commented Apr 4, 2020

karthikkeyan commented Mar 13, 2019 •

edited

Loading

karthikkeyan commented May 25, 2019 •

edited

Loading

millenomi May 28, 2019 •

edited

Loading