Skip to content

Implement URLSessionConfiguration.ephemeral #2125

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
May 13, 2019
Merged

Implement URLSessionConfiguration.ephemeral #2125

merged 1 commit into from
May 13, 2019

Conversation

pushkarnk
Copy link
Member

The difference between default and ephemeral URLSessionConfigurations
is in the storage aspect. There are three kinds of stores related to a URLSession

  • a URLCache
  • a URLCredentialStorage
  • an HTTPCookieStorage

While the default configuration mandates persistent stores, the ephemeral
configuration mandates only in-memory, ephemeral stores. As of today, URLCache
and URLCredentialStorage are unimplemented and not configured in the default
configuration. HTTPCookieStorage is implemented and we use the shared storage
in the default configuration.

This commit includes:

  • an ephemeral HTTPCookieStorage that does not read from, and write to, a persistent
    store
  • an initial URLSessionConfiguration.ephemeral implementation which configures an
    ephmeral HTTPCookieStorage, but no URLCache and URLCredentialStorage

@pushkarnk
Copy link
Member Author

@swift-ci test

@pushkarnk
Copy link
Member Author

@karthikkeyan Could you please review this?

@pushkarnk
Copy link
Member Author

@swift-ci please test

karthikkeyan
karthikkeyan reviewed Apr 17, 2019
View reviewed changes
Foundation/HTTPCookieStorage.swift
if let range = bundleName.range(of: ".", options: .backwards, range: nil, locale: nil) {
bundleName = String(bundleName[..<range.lowerBound])
}
let cookieFolderPath = _CFXDGCreateDataHomePath()._swiftObject + "/" + bundleName
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still in the learning phase, ignore this comment if it is wrong,

After looking the code from TestHTTPCookieStorage.test_cookieInXDGSpecPath(), it seems you are missing,

if let xdg_data_home = getenv("XDG_DATA_HOME") {
   // XDG Path
} else {
   // Non XDG Path
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@karthikkeyan For ephemeral storage, we don't persist the cookies in a file. The XDG spec standardises some common locations on the file system. It's not relevant if we don't want to store cookies to a file. I hope I got your question right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late response, the statement that creates the cookie folder's path, is inside if !isEphemeral

if !isEphemeral {
    // ...

    let cookieFolderPath = _CFXDGCreateDataHomePath()._swiftObject + "/" + bundleName

    // ...
}

We can leave that discussion for different PR as the code is not relevant to the intention of this PR.

Foundation/URLSession/URLSessionConfiguration.swift
// TODO: urlCache and urlCredentialStorage should also be ephemeral/in-memory
// URLCache and URLCredentialStorage are still unimplemented
let ephemeralConfiguration = URLSessionConfiguration()
ephemeralConfiguration.httpCookieStorage = HTTPCookieStorage.ephemeralStorage()
Copy link
Contributor

Choose a reason for hiding this comment

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

I just printed all the public property of ephemeral configuration, and noticed some minor discrepancies of values between the existing implementation and yours,

  • value for httpMaximumConnectionsPerHost is 4, but it is 6 in yours
  • protocolClass array contains [_NSURLHTTPProtocol, _NSURLDataProtocol, _NSURLFTPProtocol, _NSURLFileProtocol, NSAboutURLProtocol], but it is just [SwiftFoundation._HTTPURLProtocol] in yours.

Hope this helps.

Copy link
Member Author

@pushkarnk pushkarnk Apr 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That's right. The protocolClass array contains all the default native protocol implementations that come with URLSession. These are http, ftp, file and data. We currently have only http implemented and there is a pull request for ftp too. This array will be populated as and when the respective implementations are available. Thanks for noticing this, though!

Copy link
Member Author

Choose a reason for hiding this comment

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

@karthikkeyan I checked the value for httpMaximumConnectionsPerHost on Darwin with the latest dev snapshot. It is 6.

Welcome to Apple Swift version 5.0-dev (LLVM 082dec2e22, Swift 1e5637f841).
Type :help for assistance.
  1> import Foundation
  2> URLSessionConfiguration.ephemeral.httpMaximumConnectionsPerHost
$R0: Int = 6

@pushkarnk
Copy link
Member Author

@swift-ci test Linux

@pushkarnk
Copy link
Member Author

@shahmishal The PR build seems to be failing with unrelated issues. Is it possible to do a clean build? I guess clean test isn't supported?

@pushkarnk
Copy link
Member Author

@swift-ci test

@pushkarnk
Copy link
Member Author 

/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift-corelibs-foundation/Foundation/Bridging.swift:13:8: error: no such module 'CoreFoundation'

@pushkarnk
Copy link
Member Author

@shahmishal Can you please help me do a clean build here? The failures seem unrelated

@spevans
Copy link
Contributor

spevans commented Apr 24, 2019

@swift-ci test

manojmahapatra
manojmahapatra reviewed May 2, 2019
View reviewed changes
Foundation/HTTPCookieStorage.swift Outdated
@@ -183,7 +194,7 @@ open class HTTPCookieStorage: NSObject {
}

open override var description: String {
return "<NSHTTPCookieStorage cookies count:\(cookies?.count ?? 0)>"
return isEphemeral ? "Ephemeral" : "" + "<NSHTTPCookieStorage cookies count:\(cookies?.count ?? 0)>"
Copy link

Choose a reason for hiding this comment

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

string concatenation is expensive and most of the times lesser performant to string interpolation. you could use string interpolation instead.

"\(isEphemeral ? "Ephemeral" : "")<NSHTTPCookieStorage cookies count:\(cookies?.count ?? 0)>"

Copy link
Member Author

Choose a reason for hiding this comment

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

@manojmahapatra Done. Thanks!

manojmahapatra
manojmahapatra reviewed May 2, 2019
View reviewed changes
Foundation/HTTPCookieStorage.swift Outdated
@@ -200,6 +211,9 @@ open class HTTPCookieStorage: NSObject {
}

private func updatePersistentStore() {
guard !isEphemeral else { return }
Copy link

Choose a reason for hiding this comment

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

looks like we could combine these guard statements here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@manojmahapatra I'd like to separate concerns. However, I think an if statement made more sense here. It makes the code more readable and simple. Thanks!

if isEphemeral { return }

Implement URLSessionConfiguration.ephemeral
b25ca86 
The difference between `default` and `ephemeral` `URLSessionConfiguration`s
is in the storage aspect. There are three kinds of stores related to a `URLSession`
 - a `URLCache`
 - a `URLCredentialStorage`
 - an `HTTPCookieStorage`

While the `default` configuration mandates persistent stores, the `ephemeral`
configuration mandates only in-memory, ephemeral stores. As of today, `URLCache`
and `URLCredentialStorage` are unimplemented and not configured in the `default`
configuration. `HTTPCookieStorage` is implemented and we use the `shared` storage
in the `default` configuration.

This commit includes:
 - an ephemeral `HTTPCookieStorage` that does not read from, and write to, a persistent
   store
 - an initial `URLSessionConfiguration.ephemeral` implementation which configures an
   ephmeral `HTTPCookieStorage`, but no `URLCache` and `URLCredentialStorage`
@pushkarnk pushkarnk force-pushed the urlsession-config-ephemeral branch from 3efe0d5 to b25ca86 Compare May 3, 2019 08:31
@pushkarnk
Copy link
Member Author

@swift-ci please test

@pushkarnk
Copy link
Member Author

@swift-ci please test Linux

@pushkarnk
Copy link
Member Author

@swift-ci test and merge

@swift-ci swift-ci merged commit 73a8350 into swiftlang:master May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karthikkeyan karthikkeyan karthikkeyan left review comments

@manojmahapatra manojmahapatra manojmahapatra left review comments

@ianpartridge ianpartridge Awaiting requested review from ianpartridge

@spevans spevans Awaiting requested review from spevans

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pushkarnk @spevans @karthikkeyan @manojmahapatra @swift-ci