Allows for async functions/closures to be registered as routes #64

jakaplan · 2022-01-21T04:34:55Z

Adds tests to match

jakaplan · 2022-01-21T04:36:15Z

@amomchilov For your review. I dislike how much code duplication this introduces, but I couldn't figure out a way to reduce it due to the cascading nature of async.

amomchilov

I'll look at this in more depth over the weekend.

Until then, could you help me understand when a callback should be async?

amomchilov · 2022-01-22T00:22:06Z

Sources/SecureXPC/Server/XPCServer.swift

+    // MARK: Route registration
+
+    private func checkUniqueRoute(route: XPCRoute) throws {
+        if self.routes.keys.contains(route) {


IDK how much it matters, but these two operations (checking it's not already there, and setting the new value) could be done in a single hash look-up with https://developer.apple.com/documentation/swift/dictionary/3127179-updatevalue

Eh yeah I don't think any efficiency gains matter here, I'd rather consolidate the error generation into just one place.

jakaplan · 2022-01-22T01:08:28Z

Until then, could you help me understand when a callback should be async?

When the API user wants to create a callback which uses async functions within it, which will be an increasing percentage of functions over time. Without this the caller can't have any route with a reply (or one they want an error to be propagated back from) use async functions because wrapping the code in a Task won't allow for the return value (or thrown error) to be returned.

jakaplan · 2022-01-22T11:49:44Z

A realistic example could be an XPC service with a route provided a URL to a PDF. The service needs to download and then parse the PDF to extract some data. PDFs are a notorious exploit vector, so breaking out this component into an XPC service would be prudent. Using async functions to perform the download would be easiest. And there very well could be async functions involved in the data extraction (ex. running ML inference over it to extract text embedded in images).

amomchilov · 2022-01-22T17:47:41Z

When the API user wants to create a callback which uses async functions within it, which will be an increasing percentage of functions over time. Without this the caller can't have any route with a reply (or one they want an error to be propagated back from) use async functions because wrapping the code in a Task won't allow for the return value (or thrown error) to be returned.

Ah yes, that makes perfect sense, thanks for the explanation

amomchilov · 2022-01-23T15:15:55Z

Sources/SecureXPC/Server/XPCServer.swift

@@ -137,11 +149,21 @@ public class XPCServer {
    /// - Throws: If this route has already been registered.
    public func registerRoute(_ route: XPCRouteWithoutMessageWithoutReply,
                              handler: @escaping () throws -> Void) throws {


In retrospect, I don't think throwing here was a good idea (introduced in #28).

These errors can't reasonably recovered:

"Do nothing" => the route doesn't act the way you expect

"Name it something different" => makes it useless to the callers that expect a given name

Fatal erroring here might be more appropriate, so that route-registration doesn't need a try (removing try for brevity is a non-goal for me, but in some cases an unexpected try raises a suspicion error was thrown that can't really be caught)

Let me think about this. If I make this change, will do so as part of a separate PR.

amomchilov · 2022-01-23T15:18:07Z

Sources/SecureXPC/Server/XPCServer.swift

+        self.routes[route.route] = ConstrainedXPCHandlerWithMessageWithoutReplySync(handler: handler)
+    }
+
+    /// Registers a route that has a message and can't receive a reply.


I would use more context-specific terminology:

/// Registers a route that sends a message and doesn't expect a reply.

This is similar to how below you say "expects a reply" rather than "has a reply"

Hmm, this was my attempt to be specific. It's not that it doesn't expect a reply, it very much cannot have one - it's quite literally impossible with the way the code is written.

amomchilov · 2022-01-23T15:21:44Z

Sources/SecureXPC/Server/XPCServer.swift

@@ -219,15 +271,10 @@ public class XPCServer {

    private func handleEvent(connection: xpc_connection_t, event: xpc_object_t) {
        if xpc_get_type(event) == XPC_TYPE_DICTIONARY {


Another case where my guard clause instinct tingles:

private func handleEvent(connection: xpc_connection_t, event: xpc_object_t) { // Only dictionary types (and not anything which will be interpreted as an error) should be checked for // message acceptance as errors generated by libxpc can fail to meet the acceptor's criteria guard xpc_get_type(event) == XPC_TYPE_DICTIONARY else { self.errorHandler?(XPCError.fromXPCObject(event)) return } guard self.messageAcceptor.acceptMessage(connection: connection, message: event) else { self.errorHandler?(.insecure) return } handleMessage(connection: connection, message: event) }

Hmm maybe it's just my relative lack of usage of guard, but I find this a bit harder to read.

On second thought, I do prefer this. Have updated the comment to make a bit more sense with this control flow.

amomchilov · 2022-01-23T15:22:38Z

Sources/SecureXPC/Server/XPCServer.swift

-                var reply = xpc_dictionary_create_reply(event)
-                do {
-                    try handleMessage(connection: connection, message: event, reply: &reply)
-                } catch {
-                    let error = XPCError.asXPCError(error: error, expectingOtherError: true)
-                    self.errorHandler?(error)
-                    self.replyWithErrorIfPossible(error, connection: connection, reply: &reply)
-                }
+                handleMessage(connection: connection, message: event)


This is nice 🧹

amomchilov · 2022-01-23T15:23:36Z

Sources/SecureXPC/Server/XPCServer.swift

+                }
+            }
+        } else {
+            fatalError("Server handler for route \(request.route.pathComponents) cannot be run: \(handler)")


Could you clarify this case?

By my reading, it happens if you have a XPCHandlerAsync registered, but don't pass the #available(macOS 10.15.0, *) check?

Yeah. It shouldn't be possible, but seemed prudent to explicitly fail if it was encountered. I can improve the error message?

Just explain why it doesn't makes sense, along the lines of "The route wasn't an sync route, even that's the only kind that should be possible for this system"

amomchilov · 2022-01-23T15:23:59Z

Sources/SecureXPC/Server/XPCServer.swift

+                self.handleError(error, expectingOtherError: true, connection: connection, reply: &reply)
+            }
+
+        } else if #available(macOS 10.15.0, *), let handler = handler as? XPCHandlerAsync {


Oh cool, I didn't know these compile-time and run-time checks could be combined into one statement like this. Neat!

Yeah, it's very convenient!

amomchilov · 2022-01-23T15:24:29Z

Sources/SecureXPC/Server/XPCServer.swift

+                    // If a dictionary reply exists, then the message expects a reply to be sent back
+                    if let reply = reply {
+                        xpc_connection_send_message(connection, reply)
+                    }


Should this be moved out of this do block and after the catch below?

NVM, I think my bias towards minimal do blocks comes from Java/Ruby/etc. where any one of the lines can be the source of the thrown exception, and it's unclear which ones you intend to catch.

I have a new-found appreciation for try. It makes this so clear.

Should this be moved out of this do block and after the catch below?

I know you said never mind, but FWIW it needs to be in the do block as it should only run if the handler succeeds. Otherwise both this logic and the logic in the catch block would run resulting in two sends to the same reply instance which would result in XPC API Misuse.

Allows for async functions/closures to be registered as routes

74a2eef

Adds tests to match

jakaplan mentioned this pull request Jan 21, 2022

Update README.md for next version #66

Merged

amomchilov reviewed Jan 22, 2022

View reviewed changes

amomchilov approved these changes Jan 23, 2022

View reviewed changes

jakaplan and others added 3 commits January 24, 2022 18:11

Merge branch 'main' into async-server

90497a4

Updates tests to use builder style routes

dc136fe

Addresses feedback

0c43567

jakaplan merged commit 2d77b68 into main Jan 24, 2022

jakaplan deleted the async-server branch January 24, 2022 05:27

jakaplan mentioned this pull request Jan 24, 2022

Makes repeated route registration result in fatalError instead of an Error #68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows for async functions/closures to be registered as routes #64

Allows for async functions/closures to be registered as routes #64

jakaplan commented Jan 21, 2022

jakaplan commented Jan 21, 2022

amomchilov left a comment

amomchilov Jan 22, 2022

jakaplan Jan 22, 2022

jakaplan commented Jan 22, 2022 •

edited

Loading

jakaplan commented Jan 22, 2022 •

edited

Loading

amomchilov commented Jan 22, 2022

amomchilov Jan 23, 2022

jakaplan Jan 24, 2022

amomchilov Jan 23, 2022 •

edited

Loading

jakaplan Jan 23, 2022

amomchilov Jan 23, 2022

jakaplan Jan 23, 2022

jakaplan Jan 24, 2022

amomchilov Jan 23, 2022

jakaplan Jan 24, 2022

amomchilov Jan 23, 2022

jakaplan Jan 23, 2022

amomchilov Jan 24, 2022

amomchilov Jan 23, 2022

jakaplan Jan 23, 2022

amomchilov Jan 23, 2022

amomchilov Jan 23, 2022

jakaplan Jan 23, 2022

		@@ -219,15 +271,10 @@ public class XPCServer {

		private func handleEvent(connection: xpc_connection_t, event: xpc_object_t) {
		if xpc_get_type(event) == XPC_TYPE_DICTIONARY {

Allows for async functions/closures to be registered as routes #64

Allows for async functions/closures to be registered as routes #64

Conversation

jakaplan commented Jan 21, 2022

jakaplan commented Jan 21, 2022

amomchilov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakaplan commented Jan 22, 2022 • edited Loading

jakaplan commented Jan 22, 2022 • edited Loading

amomchilov commented Jan 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov Jan 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakaplan commented Jan 22, 2022 •

edited

Loading

jakaplan commented Jan 22, 2022 •

edited

Loading

amomchilov Jan 23, 2022 •

edited

Loading