Skip to content

Removing random Int in the reconnection interval of ParseLiveQuery and added warning in playgrounds #208

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

Closed
wants to merge 1 commit into from
Closed

Removing random Int in the reconnection interval of ParseLiveQuery and added warning in playgrounds #208

Show file tree
Hide file tree
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
5 changes: 5 additions & 0 deletions ParseSwift.playground/Pages/1 - Your first Object.xcplaygroundpage/Contents.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,11 @@ let scoreToFetch = GameScore(objectId: savedScore?.objectId)

//: Asynchronously (preferred way) fetch this GameScore based on it's objectId alone.
scoreToFetch.fetch { result in
/*: Warning: server does return empty object {} instead of a ParseServer error `.objectNotFount`
Copy link
Contributor

Choose a reason for hiding this comment

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

nits, change .objectNotFount to .objectNotFound

Copy link
Contributor

@cbaker6 cbaker6 Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Change:

User query.first() of query.find()

To: “Use query.first() or query.find()”

if the object does not exist yet. This might result in decoding the result into a empty
struct. User `query.first()` of `query.find()` instead if your would like to recieve
and handle an `.objectNotFount` error
*/
switch result {
case .success(let fetchedScore):
print("Successfully fetched: \(fetchedScore)")
Expand Down
10 changes: 10 additions & 0 deletions ParseSwift.playground/Pages/15 - Custom ObjectId.xcplaygroundpage/Contents.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ npm start -- --appId applicationId --clientKey clientKey --masterKey masterKey -
/*: In Xcode, make sure you are building the "ParseSwift (macOS)" framework.
*/

/*: Warning: A mixed environment with both custom and server generated ObjectId
Copy link
Contributor

Choose a reason for hiding this comment

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

Change “ObjectId” to “objectId“

is not supported. SDK throws error if set to use custom objectId
but any object without defined objectId should be saved
*/

initializeParseCustomObjectId()

//: Create your own value typed `ParseObject`.
Expand Down Expand Up @@ -89,6 +94,11 @@ score.fetch { result in
switch result {
case .success(let fetchedScore):
print("Successfully fetched: \(fetchedScore)")
/*: Warning: server does return empty object {} instead of a ParseServer error `.objectNotFount`
Copy link
Contributor

Choose a reason for hiding this comment

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

same nits here

if the object does not exist yet. This might result in decoding the result into a empty
struct. User `query.first()` of `query.find()` instead if your would like to recieve
and handle an `.objectNotFount` error
*/
case .failure(let error):
assertionFailure("Error fetching: \(error)")
}
Expand Down
5 changes: 3 additions & 2 deletions Sources/ParseSwift/LiveQuery/ParseLiveQuery.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ extension ParseLiveQuery {

var reconnectInterval: Int {
let min = NSDecimalNumber(decimal: Swift.min(30, pow(2, attempts) - 1))
return Int.random(in: 0 ..< Int(truncating: min))
return Int(truncating: min)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for changing this?

Copy link
Contributor Author

@lsmilek1 lsmilek1 Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As I mentioned here, I suspect a code race as the reconnectionInterval is mostly 0 for first two attempts and resumeTask() is deallocating the delegate with URLSession.liveQuery.delegates.removeValue(forKey: self.task)

Parse-Swift/Sources/ParseSwift/LiveQuery/ParseLiveQuery.swift

Lines 229 to 249 in e3f51f4

func resumeTask(completion: @escaping (Error?) -> Void) {
synchronizationQueue.sync {
switch self.task.state {
case .suspended:
task.resume()
URLSession.liveQuery.delegates.removeValue(forKey: self.task)
URLSession.liveQuery.delegates[self.task] = self
completion(nil)
case .completed, .canceling:
URLSession.liveQuery.delegates.removeValue(forKey: self.task)
task = URLSession.liveQuery.createTask(self.url)
task.resume()
URLSession.liveQuery.delegates[self.task] = self
completion(nil)
case .running:
open(isUserWantsToConnect: false, completion: completion)
@unknown default:
break
}
}
}

Is there a reason for the random Int? Because without the random Int the LiveQuery reconnection seems to behave much more reproducible and is even reconnecting on server reset without holding the execution with a breakpoint.

Copy link
Contributor

@cbaker6 cbaker6 Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As I mentioned here, I suspect a code race as the reconnectionInterval is mostly 0 for first two attempts and resumeTask() is deallocating the delegate with URLSession.liveQuery.delegates.removeValue(forKey: self.task)

This will need more justification and discussion before a change like this can be accepted (you can open an issue to begin that discussion). The first few attempts should come out to zero because it's a random increasing interval that increases based on the amount of attempts, so zero the first 2-3 attempts makes sense and represents trying to reconnect immediately after disconnection.

Is there a reason for the random Int?

In networking you generally have some form of increasing backoff when attempting to reconnect. Imagine you had many user devices that were connected via LiveQuery. If they all were disconnected at the same time and then attempted to reconnect at the exact same intervals, you may run into a resource issue. If they all attempt to reconnect at slightly different times, you can stagger the reconnections and hopefully reconnect everyone.

I recommend changing this back to the original as even if there is a problem, this doesn't seem like the appropriate fix. The test case shows the randomInterval is doing what it's suppose to do. You can add a print statement to see the values if you want:

Parse-Swift/Tests/ParseSwiftTests/ParseLiveQueryTests.swift

Lines 506 to 518 in e3f51f4

func testReconnectInterval() throws {
guard let client = ParseLiveQuery.getDefault() else {
XCTFail("Should be able to get client")
return
}
for index in 0 ..< 50 {
let time = client.reconnectInterval
XCTAssertLessThan(time, 30)
XCTAssertGreaterThan(time, -1)
client.attempts += index
}
client.close()
}

Copy link
Contributor

@cbaker6 cbaker6 Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If zeros are the problem, you can make the following change:

let min = NSDecimalNumber(decimal: Swift.min(30, pow(2, attempts)))
return Int.random(in: 1 ..< Int(truncating: min))

You will also need to change this line in the test to XCTAssertGreaterThan(time, 0) :

Parse-Swift/Tests/ParseSwiftTests/ParseLiveQueryTests.swift

Line 589 in 58802f5

XCTAssertGreaterThan(time, -1)

You can try that with the recent updates in #209

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for misunderstanding, the 0 was my focus and I assumed that without the random Int it is still increasing backoff, so that was the reason for that change. But I tested your update #209 and I can confirm that it solved even better the reconnection issue that should this commit resolve.

}

func resumeTask(completion: @escaping (Error?) -> Void) {
Expand Down Expand Up @@ -577,9 +577,10 @@ extension ParseLiveQuery {
completion(error)
}
} else {
let delay = isUserWantsToConnect ? 0 : reconnectInterval
self.synchronizationQueue
.asyncAfter(deadline: .now() + DispatchTimeInterval
.seconds(reconnectInterval)) {
.seconds(delay)) {
self.attempts += 1
self.resumeTask { _ in }
let error = ParseError(code: .unknownError,
Expand Down