Fix TCP connection issues #793
Conversation
kean
force-pushed
the
fix/tcp-connection-issue
branch
from
8107d61
to
eacdb61
Compare
| var additionalHeaders: [String: AnyObject] = [:] | ||
| if let oAuthToken = self.oAuthToken { | ||
| if let oAuthToken { |
There was a problem hiding this comment.
I don't know if this is required for background sessions? Probably not.
| }() | ||
|
|
||
| private func sessionConfiguration(background: Bool) -> URLSessionConfiguration { | ||
| let configuration = background ? URLSessionConfiguration.background(withIdentifier: self.backgroundSessionIdentifier) : URLSessionConfiguration.default | ||
| private static var urlSession = URLSession(configuration: .default) |
There was a problem hiding this comment.
TBD: apply the same treatment to .org and XMLRPC.
| for session in [urlSession, uploadURLSession] { | ||
| session.finishTasksAndInvalidate() | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
Description
Related PR wordpress-mobile/WordPress-iOS#23035
ℹ Please replace the above with a link to the issue this pull request addresses, as well as a summary of the implementation details.
Testing Details
ℹ Please replace this with a clear and concise description of the steps required to validate this pull request.
CHANGELOG.mdif necessary.