Skip to content
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.

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

Using a concurrent DispatchQueue #92

Closed
jeff-h opened this issue May 2, 2022 · 13 comments
Closed

Using a concurrent DispatchQueue #92

jeff-h opened this issue May 2, 2022 · 13 comments

Comments

@jeff-h
Copy link

jeff-h commented May 2, 2022

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
Copy link
Contributor

jakaplan commented May 3, 2022

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
Copy link
Author

jeff-h commented May 3, 2022

It seems to me that SecureXPC provides:

  • a Swift concurrency API for both the client (suitable for 10.15+) and the server handlers (unfortunately only Monterey+ thanks to Apple)
  • a closure-based API (suitable for 10.10+) but only for the client—there's no provision for handlers to call a completion callback, I think?

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
Copy link
Contributor

jakaplan commented May 3, 2022

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
Copy link
Author

jeff-h commented May 3, 2022

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
Copy link
Contributor

jakaplan commented May 3, 2022

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
Copy link
Author

jeff-h commented May 3, 2022

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
Copy link
Author

jeff-h commented May 3, 2022

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
Copy link
Author

jeff-h commented May 3, 2022

🎉 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
Copy link
Contributor

jakaplan commented May 4, 2022

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
Copy link
Contributor

@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
Copy link
Author

jeff-h commented May 12, 2022

Trying it now.

@jeff-h
Copy link
Author

jeff-h commented May 12, 2022

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

  • from what I gather the concurrency options are all-or-nothing. In my particular use-case it's possible I would need most requests to run concurrently, and some requests to run serially. However, regardless of the capabilities of the server, my instinct would be to take care of any serial requirement on the client side. My views on all of this are probably heavily influenced by my day job (web development) :)
  • the fact that handlers are run according to the overall server's handlerConcurrency property makes sense, but I think there's probably an implication that how they're registered (synchronous or async) would impact this. Perhaps a line in the documentation is all it would take to clarify that though.

@jakaplan
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants