Neo4j-Swift / Neo4j-Swift

Open Source Neo4j driver for iOS, tvOS, Linux and macOS
MIT License
88 stars 13 forks source link

Asynchronous calls are not asynchronous #6

Open ivoleko opened 4 years ago

ivoleko commented 4 years ago

As far as I can see. On iOS, every query I try to call, like:

are not asynchronous. Everything runs on main thread (which causes completely freeze of UI) and callback is called before function is finished.

niklassaers commented 4 years ago

Hi, that sounds weird. When they were developed they were very much asynchronous.

But, the current version is quite a bit behind the current dev version. First, I made it work with SwiftNIO, then SwiftNIO2, and building upon that I'm working on the Bolt v3 implementation. Sorry for having let integration back to master slip so far behind. I think the best path forward is to complete that work. On the plus side, it is very much asynchronous :-)

niklassaers commented 4 years ago

Hi, I've just pushed an version to the dev/non-blocking branch, that together with the dev/bolt-v3 branch of Bolt-swift should solve the asynchronous issues you were seeing. Could you check it out and confirm that the problem is solved? If not, I'm planning to move it to stable soon, and you can check it there.

niklassaers commented 4 years ago

5.2.0 has been released, would you mind testing it with that?

joshjacob commented 2 years ago

@niklassaers I think I see what the issue is. In BoltClient, when you are creating promises in the async functions, they are never being returned so the calling function would have to block in order to make sure they finish. In the case of using this in a framework like Vapor which relies on SwiftNIO, the caller would have to use GCD which bypasses the NIO efficiencies.

Since you already had the Bolt communication working with NIO, I forked and started making changes to be more strict to NIO throughout. The first and only change in Bolt-swift's Connection.swift is:

public func request(_ request: Request) throws -> EventLoopFuture<[Response]>?

to

public func request(_ request: Request) -> EventLoopFuture<[Response]>

Since NIO has a mechanism for errors, you can avoid throwing. I also removed the optional to make the downstream chaining easier.

The next set of changes I'm working on is to Neo4j-Swift ClientProtocol.swift and implementing classes. The overall strategy is to have the "async" functions utilize NIO and return EventLoopFutures and for "sync" functions to keep with Results. The NIO chaining works well when using a framework like Vapor. I can provide some sample code for this as well.

I've just started this so there is much more to do with regards to transactions and testing. Here are my forks if you wanted to review:

Feedback welcome.

– josh

niklassaers commented 2 years ago

Hi Josh, Thank you very much for making these changes. I’m no expert on NIO, so getting this feedback and changes is wonderful. Keep it up, I’ll totally support you in this, and the changes you to Bolt-swift look good to me. Thanks especially for not having to rely on GCD going forward :-)

About Neo4j-Swift/BoltClient, though, I was planning to get rid of the completion blocks too, but not for the same reasons, I think. I wanted the v6 version to use async/await, and thus just return the result instead of delivering the result via a completion block. I’m afraid that returning EventLoopFutures will surface too much of NIO and make the client harder to use for people. Last I checked with Vapor I had the impression that project wanted to move this way too, not sure how far they are down that path or if things have changed. What do you think?

Cheers

Niklas

On 17 Jan 2022, at 00.23, Josh Jacob @.***> wrote:

@niklassaers https://github.com/niklassaers I think I see what the issue is. In BoltClient, when you are creating promises in the async functions, they are never being returned so the calling function would have to block in order to make sure they finish. In the case of using this in a framework like Vapor which relies on SwiftNIO, the caller would have to use GCD which bypasses the NIO efficiencies.

Since you already had the Bolt communication working with NIO, I forked and started making changes to be more strict to NIO throughout. The first and only change in Bolt-swift's Connection.swift is:

public func request(_ request: Request) throws -> EventLoopFuture<[Response]>?

to

public func request(_ request: Request) -> EventLoopFuture<[Response]>

Since NIO has a mechanism for errors, you can avoid throwing. I also removed the optional to make the downstream chaining easier.

The next set of changes I'm working on is to Neo4j-Swift ClientProtocol.swift and implementing classes. The overall strategy is to have the "async" functions utilize NIO and return EventLoopFutures and for "sync" functions to keep with Results. The NIO chaining works well when using a framework like Vapor. I can provide some sample code for this as well.

I've just started this so there is much more to do with regards to transactions and testing. Here are my forks if you wanted to review:

https://github.com/joshjacob/Bolt-swift https://github.com/joshjacob/Bolt-swift https://github.com/joshjacob/Neo4j-Swift/tree/dev/5.2.1 https://github.com/joshjacob/Neo4j-Swift/tree/dev/5.2.1 Feedback welcome.

– josh

— Reply to this email directly, view it on GitHub https://github.com/Neo4j-Swift/Neo4j-Swift/issues/6#issuecomment-1013976517, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABATZ4A4IFN54WQE2JJCI3UWNHNTANCNFSM4NJMTN2Q. You are receiving this because you were mentioned.

joshjacob commented 2 years ago

@niklassaers Yes, adopting async/await is a great goal. The latest version of Vapor does support both (async/await and EventLoopFutures). My personal project hasn't migrated to async/await so I opted to sort things out with NIO first.

I believe the migration from NIO to async/await is pretty straightforward. With that, I believe the BoltClient class would be able to collapse the two-function pattern into a single async function. Is that your thought as well?

I'll keep working with the assumption that the NIO changes I'm making could be in a Neo4j-Swift v. 5.x release that would work for projects older than Swift 5.5, and that the conversion to async/await would be in a Neo4j-Swift v. 6.x release. How does that sound?

– josh

niklassaers commented 2 years ago

I think making it work for your personal project is very important. How interesting that Vapor supports both.

I think your suggestion to make a Neo4j-Swift v5 release with EventLoopFutures and then async/await for v6.0 is a good idea. Let’s go with that :-)

On 17 Jan 2022, at 17.32, Josh Jacob @.***> wrote:

@niklassaers https://github.com/niklassaers Yes, adopting async/await is a great goal. The latest version of Vapor does support both (async/await and EventLoopFutures). My personal project hasn't migrated to async/await so I opted to sort things out with NIO first.

I believe the migration from NIO to async/await is pretty straightforward. With that, I believe the BoltClient class would be able to collapse the two-function pattern into a single async function. Is that your thought as well?

I'll keep working with the assumption that the NIO changes I'm making could be in a Neo4j-Swift v. 5.x release that would work for projects older than Swift 5.5, and that the conversion to async/await would be in a Neo4j-Swift v. 6.x release. How does that sound?

– josh

— Reply to this email directly, view it on GitHub https://github.com/Neo4j-Swift/Neo4j-Swift/issues/6#issuecomment-1014717935, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABATZZVGU3RFHDIH64G4DTUWRACXANCNFSM4NJMTN2Q. You are receiving this because you were mentioned.