trilemma-dev/SecureXPC

Add support for handling client errors on reply-less messages

Closed this issue · 7 comments

amomchilov commented

When there's no "reply" block, there's nowhere to detect errors that were encountered after sending a reply-less message.

jakaplan commented

I agree with the problem. Will leave feedback on the PR in regards to the specific solution.

amomchilov commented

Response to https://github.com/trilemma-dev/SecureXPC/pull/18/files#r748837217

Hmmm you're totally right.

Seems like a way to ensure consistency here is under the hood to always send XPC messages that expect a reply and then have the send(...) functions that don't have a reply be able to provide an error handler. Or potentially as a bonus make it a completion handler so that it's possible for the client to get confirmation the message was received.

I experimented with this idea. One thing to note is that you can't just use a Result<(), XPCError> reply type as usual (because Void isn't Codable).

Or potentially as a bonus make it a completion handler so that it's possible for the client to get confirmation the message was received.

I think this would be good. The completion handler can be ((XPCError?) -> Void)?. When it's nil, we can optimize and use the reply-less API.

Aside from the consistency issues, the approach in this PR makes it impossible (or at least inconvenient?) for the client to know which send(...) calls didn't succeed.

True. I didn't notice this because I was narrowly interested in seeing the connected/disconnected status of my connection (I built a small little debugging UI in my App using NSImageNameStatusNone/NSImageNameStatusAvailable/NSImageNameStatusUnavailable indicators)

jakaplan commented

I experimented with this idea. One thing to note is that you can't just use a Result<(), XPCError> reply type as usual (because Void isn't Codable).

Why does the reply type need to conform to Codable in this case?

Right now the reply handler is public typealias XPCReplyHandler<R> = (Result<R, XPCError>) -> Void which has no requirement the success type be Codable. The send(...) and sendMessage(...) functions that take replies have a constraint of R: Decodable, but the two functions without replies wouldn't need to. Also we don't have to use this same typealias (and if we do, we should rename it to something more generic so it doesn't only make sense for replies.

I think this would be good. The completion handler can be ((XPCError?) -> Void)?. When it's nil, we can optimize and use the reply-less API.

Hmm, not sure we should actually allow it to be nil considering that makes it easy to ignore thrown errors. Seems rather against the Swift's overall error handling paradigm.

amomchilov commented

@jakaplan

Why does the reply type need to conform to Codable in this case?

Although the reply-less send/sendMessage wouldn't have the R: Decodable constraint, they both end up forwarding to sendWithReply, which does. Unless we added a new send method which takes care of it.

Seems rather against the Swift's overall error handling paradigm.

On the contrary, Swift has a whole suite of "escape hatches" (!, try!, as!, unsafeBitCast, etc.). I haven't nailed down the exact design for my app <-> helper communications, but one part of it has a UDP-like quality: if the message didn't make it the first time, it's too late to resend it (it would be useless if it's late), and there's not much error handling I could do.

jakaplan commented

Although the reply-less send/sendMessage wouldn't have the R: Decodable constraint, they both end up forwarding to sendWithReply, which does. Unless we added a new send method which takes care of it.

I imagine it'd be a function which "wraps" the existing sendWithReply functional or potentially this is directly done inside of the send and sendMessage functions depending on how much code is involved.

On the contrary, Swift has a whole suite of "escape hatches" (!, try!, as!, unsafeBitCast, etc.).

Those don't let you ignore the error though, they terminate your process if an error actually occurs. If I understood you correctly passing in nil would mean nothing occurs on the client in response to an error being thrown by the server.

amomchilov commented

I imagine it'd be a function which "wraps" the existing sendWithReply functional or potentially this is directly done inside of the send and sendMessage functions depending on how much code is involved.

I'm looking into it. First I was trying to decide if this warrants new route types.

If I understood you correctly passing in nil would mean nothing occurs on the client in response to an error being thrown by the server.

That's pretty much equal to _ = try? somethingThatMightThrow

jakaplan commented

That's pretty much equal to try? somethingThatMightThrow (where the resulting value is not used

Good point, I so rarely use try? I forgot it exists. Actually while I've never done this before, it turns out try? can also be used when calling a function that has no return value. This is perfectly valid:

let client = XPCClient.forMachService(named: "com.example.service")
let route = XPCRouteWithoutMessageWithoutReply("configamajig", "discombobulate")
try? client.send(route: route)

So with that in mind, sure passing in nil is fine to ignore any error.