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.

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

Fix TCP connection issues #793

Closed
wants to merge 1 commit into from
Closed

Fix TCP connection issues #793

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 16 additions & 22 deletions Sources/CoreAPI/WordPressComRestApi.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,6 @@ open class WordPressComRestApi: NSObject {
super.init()
}

deinit {
for session in [urlSession, uploadURLSession] {
session.finishTasksAndInvalidate()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One side effect is ongoing tasks won't be invalidated at all. I wonder if we should make WordPressComRestApi accept a URLSession instance, which the app can create upon use signing in and invalidate upon logout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a need to invalidate it? The requests are signed on a per-request basis now.

Something does need to change with the WordPressComRestApi/invalidateAndCancelTasks usage. The lifetime of _wordPressComRestApi is tied to a WPAccount, which is also problematic: there is one instance per thread, and the instances could get reset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to invalidate it?

Good question. I don't know for sure though. Using a shared URLSession singleton between user sessions is a change introduced in this PR, which we'll need to test to make sure it doesn't break things.

IMO, the best practice is URLSession instance should be per user session. Upon user logs in, they get a new URLSession instance and uses it until logs out. Once logs out, the URLSession instance should be destroyed. That's similar to how Safari uses one URLSession for each tab.

The lifetime of _wordPressComRestApi is tied to a WPAccount, which is also problematic: there is one instance per thread, and the instances could get reset.

Yes, definitely. That's another big piece of tech debt.

Copy link
Contributor Author

@kean kean Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, the best practice is URLSession instance should be per user session. Upon user logs in, they get a new URLSession instance and uses it until logs out. Once logs out, the URLSession instance should be destroyed. That's similar to how Safari uses one URLSession for each tab.

I completely agree. The app should have a repository that could control the lifetime of the objects to match the lifetime of the user session. It will be a better fix for this issue as well (assuming it actually fixes it because I don't have any hard evidence yet).


/// Cancels all outgoing tasks asynchronously without invalidating the session.
public func cancelTasks() {
for session in [urlSession, uploadURLSession] {
Expand Down Expand Up @@ -321,6 +315,12 @@ open class WordPressComRestApi: NSObject {
builder = builder.query(defaults: [URLQueryItem(name: localeKey, value: preferredLanguageIdentifier)])
}

if let oAuthToken {
builder = builder.header(name: "Authorization", value: "Bearer \(oAuthToken)")
}
if let userAgent {
builder = builder.header(name: "User-Agent", value: userAgent)
}
return builder
}

Expand All @@ -333,34 +333,28 @@ open class WordPressComRestApi: NSObject {

// MARK: - Async

private lazy var urlSession: URLSession = {
URLSession(configuration: sessionConfiguration(background: false))
}()
private var urlSession: URLSession { WordPressComRestApi.urlSession }

private lazy var uploadURLSession: URLSession = {
let configuration = sessionConfiguration(background: backgroundUploads)
configuration.sharedContainerIdentifier = self.sharedContainerIdentifier
if configuration.identifier != nil {
return URLSession.backgroundSession(configuration: configuration)
} else {
return URLSession(configuration: configuration)
if backgroundUploads {
return makeBackgroundSession()
}
return WordPressComRestApi.urlSession
}()

private func sessionConfiguration(background: Bool) -> URLSessionConfiguration {
let configuration = background ? URLSessionConfiguration.background(withIdentifier: self.backgroundSessionIdentifier) : URLSessionConfiguration.default
private static var urlSession = URLSession(configuration: .default)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBD: apply the same treatment to .org and XMLRPC.


private func makeBackgroundSession() -> URLSession {
let configuration = URLSessionConfiguration.background(withIdentifier: self.backgroundSessionIdentifier)
var additionalHeaders: [String: AnyObject] = [:]
if let oAuthToken = self.oAuthToken {
if let oAuthToken {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this is required for background sessions? Probably not.

additionalHeaders["Authorization"] = "Bearer \(oAuthToken)" as AnyObject
}
if let userAgent = self.userAgent {
if let userAgent {
additionalHeaders["User-Agent"] = userAgent as AnyObject
}

configuration.httpAdditionalHeaders = additionalHeaders

return configuration
return URLSession.backgroundSession(configuration: configuration)
}

func perform(
Expand Down