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

lsmilek1 · 2021-08-02T15:51:26Z

Added warning in playgrounds related to:

mixed environment with both custom and server generated objectId not supported
ParseObject.fetch() returns empty object instead of .objectNotFound ParseError

Removing random Int in the reconnection interval of ParseLiveQuery:

If socket is being open by user, there is no reason for delay -> Added boolean check

- mixed environment with both custom and server generated objectId not supported - ParseObject.fetch() returns empty object instead of .objectNotFound ParseError Removing random Int in the reconnection interval of ParseLiveQuery If socket is being open by user, there is no reason for delay -> Added boolean check

codecov · 2021-08-02T15:58:25Z

Codecov Report

Merging #208 (58802f5) into main (e3f51f4) will increase coverage by 0.07%.
The diff coverage is 84.00%.

❗ Current head 58802f5 differs from pull request most recent head 39e92a9. Consider uploading reports for the commit 39e92a9 to get more accurate results

@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
+ Coverage   82.76%   82.84%   +0.07%     
==========================================
  Files          76       76              
  Lines        7242     7250       +8     
==========================================
+ Hits         5994     6006      +12     
+ Misses       1248     1244       -4

Impacted Files	Coverage Δ
Sources/ParseSwift/LiveQuery/ParseLiveQuery.swift	`72.36% <80.95%> (+1.25%)`	⬆️
Sources/ParseSwift/LiveQuery/LiveQuerySocket.swift	`44.28% <100.00%> (+1.63%)`	⬆️
Sources/ParseSwift/Objects/ParseObject.swift	`78.09% <0.00%> (-0.26%)`	⬇️
Sources/ParseSwift/Objects/ParseUser.swift	`82.89% <0.00%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3f51f4...39e92a9. Read the comment docs.

cbaker6 · 2021-08-02T15:56:41Z

Sources/ParseSwift/LiveQuery/ParseLiveQuery.swift

    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)


What's the reason for changing this?

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.

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()

}

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

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.

cbaker6 · 2021-08-02T15:58:02Z

ParseSwift.playground/Pages/1 - Your first Object.xcplaygroundpage/Contents.swift


 //: 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`


nits, change .objectNotFount to .objectNotFound

Change:

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

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

cbaker6 · 2021-08-02T15:58:38Z

ParseSwift.playground/Pages/15 - Custom ObjectId.xcplaygroundpage/Contents.swift

    switch result {
    case .success(let fetchedScore):
        print("Successfully fetched: \(fetchedScore)")
+    /*: Warning: server does return empty object {} instead of a ParseServer error `.objectNotFount`


same nits here

cbaker6 · 2021-08-02T20:07:06Z

@lsmilek1 thanks for submitting your first PR! Don't forget to add a change log entry: https://github.com/parse-community/Parse-Swift/blob/main/CHANGELOG.md

cbaker6 · 2021-08-03T02:25:59Z

ParseSwift.playground/Pages/1 - Your first Object.xcplaygroundpage/Contents.swift


 //: 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`


Change:

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

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

cbaker6 · 2021-08-03T02:27:14Z

ParseSwift.playground/Pages/15 - Custom ObjectId.xcplaygroundpage/Contents.swift

 /*: In Xcode, make sure you are building the "ParseSwift (macOS)" framework.
 */

+/*: Warning: A mixed environment with both custom and server generated ObjectId


Change “ObjectId” to “objectId“

lsmilek1 · 2021-08-03T06:56:56Z

Closing this PR as there the LiveQuery reconnection is solved with #209. Thank you!

cbaker6 suggested changes Aug 2, 2021

View reviewed changes

cbaker6 reviewed Aug 3, 2021

View reviewed changes

lsmilek1 closed this Aug 3, 2021

	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
	}
	}
	}

	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()
	}

Uh oh!

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

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

Uh oh!

Conversation

lsmilek1 commented Aug 2, 2021

Uh oh!

codecov bot commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cbaker6 Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

lsmilek1 Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbaker6 Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbaker6 Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsmilek1 Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

cbaker6 Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

cbaker6 Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbaker6 Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

cbaker6 commented Aug 2, 2021

Uh oh!

cbaker6 Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbaker6 Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

lsmilek1 commented Aug 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Aug 2, 2021 •

edited

Loading

lsmilek1 Aug 2, 2021 •

edited

Loading

cbaker6 Aug 2, 2021 •

edited

Loading

cbaker6 Aug 2, 2021 •

edited

Loading

cbaker6 Aug 3, 2021 •

edited

Loading

cbaker6 Aug 3, 2021 •

edited

Loading