Async API+Command

[cbaker6]

Add async API, all networking calls are pushed to URLSession.shared, which uses the necessary threads carry out the networking request.

This PR adds/changes the following:

• Modified Async API - previous version went async->sync->async, now it's async
• Modified Sync API - previous version used URLSession directly, this one syncs the executeAsync now
• Added callbackQueue to async API calls. When async calls are made, the developer can specify the queue to return the call on. It defaults to the main queue if no queue is specified
• Modernized Async and batch calls so they return a Result type for better data/error handling
• Bug fixes to batching
• Bug fixes when saving a response from the server
• Made the framework easier to test by marking some value types "internal" and "Codable" when needed
• Testcases for most of the async calls*

*The test cases are on this branch. They will be added after #14 is merged as they are dependent on URLMocking.

[mtrezza]

There's an issue because the async session gets killed, since it's not synchronous and nothing is holding the schedule, killing the session. The current way is still blocking the calling thread, allowing the async to finish.

Can you give more details about the issue and where in code it occurs?

[cbaker6]

@pranjalsatija let me know if you have some thoughts on how to fix this? I know you and Flo were discussing API which led to the current version.

The sync version of “async” works, so it’s possible there’s a quick fix

[pranjalsatija]

I took a look, and the thing that sticks out the most to me is the fact that the async version completely reimplements the sync version. I think it would be much easier to just wrap the sync version in a call to DispatchQueue.async and call it a day. I have a new branch on my repo called async_api that you can check out if you'd like, where I've implemented those changes. Basically, I added a queue: DispatchQueue property to API.Command where each queue gets a label that's randomly generated with UUID. All async calls just do queue.async and wrap the sync versions.

I don't have the test case you mentioned, but if you want to post it here or run it on your local copy, I think this would solve the issue.

[cbaker6]

I took a look, and the thing that sticks out the most to me is the fact that the async version completely reimplements the sync version. I think it would be much easier to just wrap the sync version in a call to DispatchQueue.async and call it a day. I have a new branch on my repo called async_api that you can check out if you'd like, where I've implemented those changes. Basically, I added a queue: DispatchQueue property to API.Command where each queue gets a label that's randomly generated with UUID. All async calls just do queue.async and wrap the sync versions.

I tried something similar at first, but it didn't work work. It's possible I set it up wrong. I'll take a further look at your branch and see if I can test it

[cbaker6]

I think it would be much easier to just wrap the sync version in a call to DispatchQueue.async and call it a day.

I'm still looking, but the problem with this is that URLSession is async by design and made in a way to not require Dispatch. The current synchronous design of execute turns the asynchronous URLSession into synchronous. By wrapping, we are essentially going from "async->sync->async". Removing the semaphores in the executeAsync allows us to leverage the already async URLSession, but it doesn't work, I simply have the semaphores there to ensure that it code works, not to reimplement the original one

[cbaker6]

@pranjalsatija I'm not able to see your async_api branch. Did you push it too GitHub?

[pranjalsatija]

Just did! Sorry about that.

[cbaker6]

Just did! Sorry about that.

@pranjalsatija I don't think your updates were committed. The last commit I see is from this branch

[pranjalsatija]

🤦‍♂️ Fixed it. My bad.

[cbaker6]

@pranjalsatija the changes you made has the same type of issues I mentioned on here. Also be sure to look at my note here. When we finally fix the async case, we can wrap that one to make the sync case, but I don't think it should be the other way around for the reasons I stated in the comment. I left the current sync case intact as it works and I didn't want to break it, which is why I copied portions of it.

You can test your code with the test case I made in this branch https://github.com/netreconlab/Parse-Swift/tree/async_other

The test case is here: https://github.com/netreconlab/Parse-Swift/blob/9032cc370044a8e63559297c9886268829b21913/Tests/ParseSwiftTests/ParseObjectCommandTests.swift#L231

You can set a break point here: https://github.com/netreconlab/Parse-Swift/blob/9032cc370044a8e63559297c9886268829b21913/Tests/ParseSwiftTests/ParseObjectCommandTests.swift#L251

If you hit that break point without using the semaphore/wait, etc. then you made async work.

[pranjalsatija]

Have you tried using expectations in the test case? I'm not on my laptop right now but a quick glance at the test code shows that there's no waitForExpectation which might explain why the breakpoint isn't getting hit: Xcode is terminating the test because it doesn't know it's supposed to wait for an asynchronous task to complete.

[pranjalsatija]

If the test shows up in Xcode as passed but the breakpoint isn't getting hit, that's almost certainly the case.

[cbaker6]

@pranjalsatija I haven't tried that, but will now, thanks!

[pranjalsatija]

Sounds good!

And about your earlier comment regarding sync to async back to sync: I agree. If the new async implementation works, we can keep it and implement the sync functionality using the async implementation.

[cbaker6]

@pranjalsatija this worked for the branch I made for your code. It didn't work on my original async branch though.

My initial thinking is that this is moving the networking off of the com.parse.ParseSwift.async queue and into a thread labeled by the UUID you made (which behaves synchronously), but since it's off the com.parse.ParseSwift.async, API can still process more calls (this might be okay). If we want to serialize all of the networking calls, we can move them to a dedicated networking thread instead of the UUID to ensure they are processed in the order they are made (this would behave similar to the current execute.

I will look into this more, thanks for your help!

Let me know if you have anymore ideas

Update, it works on the original async case I have for iOS 12-, but not the iOS13 case using Combine

[pranjalsatija]

I agree, I just have one or two things to add:

• The networking is synchronous, but since each command gets its very own DispatchQueue, I don't think it'll limit concurrency or cause any other problems. Even so, if the plan is to use async networking by default and implement sync methods using the async ones, this won't matter anyway.
• I think moving all networking operations onto a single queue just for networking is a decent idea, as long as we document it. I think it'll make concurrency easier later on when we start getting into local data storage and other related functionality.

[cbaker6]

The networking is synchronous, but since each command gets its very own DispatchQueue, I don't think it'll limit concurrency or cause any other problems

I think you are right here in a that since all calls to the network are using URLSession.shared, so these are executed by URLSession as they are received. There’s currently only one queue for all commands which is labeled, com.parse.ParseSwift.async In Asynchronous.swift. I would imaging appending a UUID on the end will allow for multiple command threads if we really needed that which “might” work out okay still since URLSession.shared is the only session that is handling all networking (seems like it calls the background thread when needed for file downloads) and knows how to return back to the correct queue. This can possibly create a lot queues though. The current modification works as the test cases test each async situation by spawning 100 concurrent threads

[cbaker6]

Made changes to get this working (thanks again @pranjalsatija for the test case expectation, that was definitely the issue). execute now leverages executeAsync to reduce duplicate code. I've added all of the test cases in a different branch and will bring all of the code over to this one once PR 13 is merged. After adding that code, this branch is ready for review...

A note about using Combine publisher/subscriber. This versions of the code doesn't use it, mainly because it looks like the subscriber gets killed when it goes out scope (the issues I was mentioning at the beginning of the thread). The publisher is still alive and fires Future like it's suppose to, but there's no subscriber left to receive the data. This can be looked at in the future, but isn't a big deal now is the current version is still async. All it means is the we are not using the most modern swift style code which is only available for iOS13+ anyways

[pranjalsatija]

LGTM!

[cbaker6]

@TomWFox can you approve?

[drdaz]

I spent the afternoon checking this out. Looks great. Learned a few things on the way.

In fact it's been a few weeks since I looked at this project... tremendous progress all around 👏🏼👏🏼.

[drdaz]

@TomWFox can we add @cbaker6 to the objc sdk team too? If he wants to ofc.

[cbaker6]

@drdaz thanks for the kudos! @pranjalsatija reviews, feedback, and code additions have been valuable to the updates!

I wouldn't mind helping with the obj-c project though I try not to write obj-c code nowadays, I can still help with bug fixes and CI

[drdaz]

@drdaz thanks for the kudos!

I wouldn't mind helping with the obj-c project though I try not to write obj-c code nowadays, I can still help with bug fixes and CI

I also prefer not to write obj-c. It hurts my brain and eyes way more than necessary 😃.

But I could really use an active pair of eyes over there rn.

[cbaker6]

I also prefer not to write obj-c. It hurts my brain and eyes way more than necessary 😃.

same for me!

I do use the obj-c SDK in my Swift apps heavily, so I'm definitely willing to help out

[TomWFox]

@drdaz added to my mental to do list