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

Client-level error handling #67

Closed
amomchilov opened this issue Jan 23, 2022 · 6 comments
Closed

Client-level error handling #67

amomchilov opened this issue Jan 23, 2022 · 6 comments

Comments

@amomchilov
Copy link
Contributor

amomchilov commented Jan 23, 2022

One thing that I've noticed in my use for SecureXPC was that all my clients' callbacks followed the same pattern: they all switch on the result, and called a shared error handling function (in addition to some route-specific error handling).

I think this might be a common use case, especially because changes to the connection state might want to be monitored, and that can happen on every call.

Adding a "global" error handler that applies across all routes of a client would be easy, but then you'd get duplicate reports of errors (once per callback, and then once again). Ideally, having a way to express "if the route callback doesn't handle the error, then give the client error handle a chance to handle it", but I don't know how we could detect if the callback handled the error or not.

Do you have any ideas for how we could improve this?

@jakaplan
Copy link
Contributor

Hmm, I haven't experienced the same thing. This may be because I don't have very many routes and for some of my routes I want considerably different error handling behavior.

When I've written networking code using NSURL I found the centralization of reported errors via delegates to be quite convoluted. It's true there are some benefits to it, but overall I found myself needed to work around it to "re-associate" the errors with the actual request I made in order to determine what action - if any - to take.

I'm not sure trying to address this at the framework level would be a good idea. I mean that literally, and not as indirect way of saying "I think this is a bad idea". My gut reaction is attempting to do so would introduce a new set of issues.


Adding a "global" error handler that applies across all routes of a client would be easy, but then you'd get duplicate reports of errors (once per callback, and then once again).

Indeed. And it'd be very problematic to only send errors to the "global" handler if it is set. Because then awaited functions which experienced an error just wouldn't return and that'd terribly mess up control flow. To work around that we could instead throw an error like XPCError.alreadyProvidedToGlobalHandler(XPCError), but that seems deeply non-standard. (This is equally applicable to the closure-based variants, just seemed easier to describe for the async case.)

Ideally, having a way to express "if the route callback doesn't handle the error, then give the client error handle a chance to handle it", but I don't know how we could detect if the callback handled the error or not.

I can't imagine there's any way to do that as what does it even mean to "handle the error" in any sort of deterministic sense? For example:

do {
    let reply = try await client.sendMessage("Get Schwifty", route: route)
    <# do something with reply #>
} catch {
    if case let .other(message) = error {
        print("Unable to get Schwifty because: \(message)")
    }
}

In this example the error is being caught (not that I think we can detect that?), but only one XPCError case is any way being handled - all of the rest are being ignored. I can't see how we'd able to know that.

@amomchilov
Copy link
Contributor Author

amomchilov commented Jan 23, 2022

You're right on the money, this is what I found challenging about this problem.

Here's a few different ideas I was thinking of:

  1. It ain't pretty, but we can make a whole set of new send overloads whose closure only expects the Success value of the Result (and internally the Failure error gets forwarded to the client-level error handler). But this is restrictive, because you can't express "handle some specific error about this route, then do the global error handling relating to updating my connection status indicator (or whatever)"

  2. I was also thinking of a more use-case-specific approach. The purpose of my shared error handler was to observe changes in the client state. Perhaps this should be a first-class feature of the client, to have a connectionStateDidChange: (ConnectionState) -> Void callback.

    Even then, that raises the question of whether route-handlers should also receive connection state callbacks (and if so, in which cases?). Hard to decide.

There's a few inter-related challenges:

  1. If there's both a route-level and client-level error handler, we don't always want to duplicate the errors by sending them to both

    • Duplication aside, if the client handler has to implement decision logic on every callback to decide whether or not to ignore the error (because it'll rely on the client-level error handler receiving and handling it), then it kind of defeats that purpose. That same decision logic could have been used to call myOwnSharedErroHandler(error) manually instead.
  2. What you described here:

    I found myself needing to work around it to "re-associate" the errors with the actual request I made in order to determine what action - if any - to take.

I think one thing that makes the design of this challenging for me is my ignorance on exactly when a route handler should need to know about connection statuses (this will improve as I get more real-world experience using this framework). Retries sound like one use case that would need it. Retries would a useful (essential?) feature, but IDK what architectural layer should be responsible for implementing them. For one, if we want to support the "send, oh no the Mach service isn't installed, install it, then retry" pattern, then that would required Blessed, so perhaps retries should be implemented at the "umbrella framework" (name TBD) level?

  • But if that's the case, then the only way I imagine that could be implemented is if the umbrella framework fully wrapped the XPCClient API, and exposed its own public APIs that handle retrying under-the-hood. This would be a pretty heavy-handed approach, but IDK what other options there might be.

Any thoughts?

@jakaplan
Copy link
Contributor

For one, if we want to support the "send, oh no the Mach service isn't installed, install it, then retry" pattern, then that would required Blessed, so perhaps retries should be implemented at the "umbrella framework" (name TBD) level?

Is this a pattern you're looking to do unconditionally? Because it requires user action, that's not how my code handles things. While in theory a Mach service could become uninstalled at any time, in practice it probably will never be — the far more likely scenario is it's never been installed. So checking that on first run (or the beginning of each run) is what makes sense to me.

@jakaplan
Copy link
Contributor

@amomchilov Six months later, what are your thoughts on whether client level error handling should exist?

@amomchilov
Copy link
Contributor Author

I'll have to think more on this.

Marking unread for now, I'll come back to this when I make up my mind xD

@jakaplan
Copy link
Contributor

jakaplan commented Feb 2, 2023

Closing due to inactivity. If desired, please open a new issue in the future with specific examples/use cases.

@jakaplan jakaplan closed this as completed Feb 2, 2023
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