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

Add support for handling client errors on reply-less messages #17

Closed
amomchilov opened this issue Nov 13, 2021 · 7 comments · Fixed by #57
Closed

Add support for handling client errors on reply-less messages #17

amomchilov opened this issue Nov 13, 2021 · 7 comments · Fixed by #57

Comments

@amomchilov
Copy link
Contributor

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

@jakaplan
Copy link
Contributor

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

@amomchilov
Copy link
Contributor Author

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

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

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

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

amomchilov commented Nov 24, 2021

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

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.

jakaplan pushed a commit that referenced this issue Jan 18, 2022
Fixes #17

All `send` and `sendMessage` functions now take in a `XPCResponseHandler` (renamed from `XPCReplyHandler`). For those that require a reply, the response handler is non-optional. For those that can't have a reply, they can optionally provide a `XPCResponseHandler<Void>` which will get called upon successful completion or be provided an `XPCError`.

In all cases, none of the functions now `throw` - all errors generated are provided to their handlers (or ignored if no handler is provided).

Two existing tests were modified and two new tests have been added.

This has been designed in a way which should make it relatively straightforward to address #19.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants