trilemma-dev / SecureXPC

A simple and secure XPC framework for Swift
MIT License
73 stars 15 forks source link

Using a concurrent DispatchQueue #92

Closed jeff-h closed 2 years ago

jeff-h commented 2 years ago

I've set my server's target queue like so:

let server = try XPCServer.forThisBlessedHelperTool()

// Either one of:
server.targetQueue = DispatchQueue.global()
server.targetQueue = DispatchQueue(label: "queuename", attributes: .concurrent)

...but calling my test handler multiple times from the client still indicates them running serially.

static func testRouteHandler(withMsg msg: String) throws -> String {
    let process = Process()
    process.launchPath = "/bin/sleep"
    process.arguments = ["4"]
    process.launch()
    process.waitUntilExit()
    return "Finished: \(msg)"
}

I noticed the following in the xpc_connection_set_target_queue documentation:

If the target queue is a concurrent queue, then XPC still guarantees that there will never be more than one invocation of the connection's event handler block executing concurrently.

Is what I'm trying to do the right approach?

jakaplan commented 2 years ago

I noticed the following in the xpc_connection_set_target_queue documentation

Ah, indeed that's the behavior you're getting as described. Using a concurrent queue with an XPCServer will cause concurrent handlers to run if the requests are from different clients, but not concurrently from the same client.

I have ideas of how to change this behavior so calls from the same client are (mostly) concurrent, but will need to give it some thought.

jeff-h commented 2 years ago

It seems to me that SecureXPC provides:

Is it feasible to add a completion callback API to the server handlers? Sounds like it would be a lot of work, and only useful for pre-Monterey :(

jakaplan commented 2 years ago

a closure-based API (suitable for 10.10+) but only for the client

The server API is exclusively closure-based. However, your issue with it is that the closures are never called concurrently for requests from the same client. If they were called concurrently, that would meet your needs. Is that a correct understanding?

jeff-h commented 2 years ago

Yes indeed, concurrently-called handlers would be great. It would also fit nicely with the client / server / routes metaphor, at least in my mind. I have to be honest — to date I assumed that the handlers I'd written using async were all running concurrently.

For what it's worth, when I said there's no closure-based API on the server side, I meant in the sense that you currently provide both async and synchronous versions of the four registerRoute funcs, but no provision for an old-school "completion callback" style of async routes. The latter would be an async handler solution suitable for pre-macOS 10.15.

Since the route sync/async status has no bearing on whether it's actually called sync/async'ly, I'm not certain how relevant the above paragraph is now though.

Sorry if I seem to have an odd perspective on all this! My app is massively Promises-based, including the previous incarnation of my helper code (it all predates Swift's new Concurrency). So when I converted my old helper to leverage SecureXPC it was all with a promises / completion-handler mentality, in which virtually everything is running asynchronously.

jakaplan commented 2 years ago

Try out this branch and let me know if this behaves the way you're expecting.


I have to be honest — to date I assumed that the handlers I'd written using async were all running concurrently.

They are (well the degree of concurrency is up to the Swift runtime to decide), it's the synchronous ones that were not. This difference in behavior was not intentional.

I meant in the sense that you currently provide both async and synchronous versions of the four registerRoute funcs, but no provision for an old-school "completion callback" style of async routes.

I'm not 100% sure what you have in mind, but I think there is actually what you describe, but may not be for your use case. There are in fact six versions of the registerRoute functions and the other two pass the registered closure a SequentialResultProvider instance which can be used at future points in time (and from any thread). However, this is intended for long running (or even indefinite) actions where the client needs the latest data as it becomes available.

jeff-h commented 2 years ago

I'm trying it now, thanks! My first attempt is returning:

failure(SecureXPC.XPCError.decodingError("valueNotFound(Foundation.UUID, Swift.DecodingError.Context(codingPath: [], debugDescription: \"Key not present: __request_id\", underlyingError: nil))"))

...after my handler completes. Just checking through my testing code to see if I'm doing something silly.

jeff-h commented 2 years ago

SwiftAuthorizationApp also returns the above error for me on its stock routes, when I upgrade its SecureXPC dependency to the concurrency branch.

For this branch, should I be setting server.targetQueue = DispatchQueue.global()?

(Although I've tried it with and without, and still get the error)

jeff-h commented 2 years ago

🎉 Horray, it works!

(would you believe I was forgetting to update the helper — I'm so used to the awesome self-updating mechanism provided by SwiftAuthorizationHelperTool/Update.swift)

This is exactly what I was hoping for. I've added two dummy routes which call these handlers:


    static func testHandlerOne(withMsg msg: String) throws -> String {
        print("starting handler one")
        sleep(4)

        return "Handler one responds with \(msg)"
    }

    static func testHandlerTwo(withMsg msg: String) throws -> String {
        print("starting handler two")

        let process = Process()
        process.launchPath = "/bin/sleep"
        process.arguments = ["4"]
        process.launch()
        process.waitUntilExit()

        return "Handler two responds with \(msg)"
    }

I can call any combination of these multiple times, rapid-fire, and after four seconds the responses from both all come back rapid-fire as expected.

This is fantastic, thanks again. Tomorrow I'll begin porting this into my app; it may take me some time as I have some handlers which leverage Swift's async so I'll need to change what I'm doing there. Still, I think this will actually simplify my code overall, as a side-benefit. And be Catalina-compatible :)

jakaplan commented 2 years ago

Tomorrow I'll begin porting this into my app; it may take me some time as I have some handlers which leverage Swift's async so I'll need to change what I'm doing there.

Great to hear it's working for you.

The branch I pointed you at was a proof of concept, it's possible the exact API which I release won't match - but it will enable the same concurrent behavior you're currently getting. At most you'd need to set one or two additional properties (if they're not the default ones) for the server.

There's a good chance I'm going to enable both unrestricted concurrency (what that branch currently always does) as well as concurrency with serial execution per client (what the released version currently for synchronous handlers, but not async handlers). Serial per client execution is useful for stateful operations, hence why I'm likely to support it as well.

jakaplan commented 2 years ago

@jeff-h Would you be up for trying out the server-concurrency branch and letting me know if it works properly for you (I hope so as I wrote some automated tests) and what you think of the API design.

There's no longer an ability to set a DispatchQueue, instead XPCServer has a handlerConcurrency property that by default is concurrent and could also be set to serialPerClient or serialPerServer. This behavior is consistent regardless of whether the registered handlers are synchronous or async.

I'm not sure which branch I'm going to go with. This one offers more customization, but I've yet to decide if it's worth the additional (framework internal) complexity.

jeff-h commented 2 years ago

Trying it now.

jeff-h commented 2 years ago

Using this new branch my app builds and runs seamlessly without any code changes, and appears to function as expected 👍

Regarding the new functionality, I'm vastly under-qualified to offer an opinion on any of it lol but nonetheless...

jakaplan commented 2 years ago

Thanks for the feedback. I've gone with the former approach along with improved documentation. This has now been incorporated into 0.7.0.