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 an escape hatch when properties fail #143

Closed
wants to merge 3 commits into from

Conversation

stevenbrix
Copy link
Collaborator

@stevenbrix stevenbrix commented Mar 4, 2024

Right now you can't easily handle property get/sets that fail. The current design would require the developer to do something like this:

let foo = try obj._default.get_Foo()

While not the worse, you then would need to know which internal interface to invoke the property on Also, the internals aren't exposed (mainly due to export limit issues). While we could fix that issue for cmake, export limits are always going to be an issue for SPM, so I think it makes sense to not require the solution to expose all of those.

Solution

Provide a withFailingCall method that developers can use if they need to gracefully handle an error. What this does is it allocates a LastErrorHolder object, which is checked whenever a WinRT method fails. If there is a LastErrorHolder, it is given the error of the winrt method call. withFailingCall then checks for the error and throws the exception. Here is the implementation of the method:

public func withFailingCall<Result>(_ call: @autoclosure () -> Result) throws -> Result {
  let error = getLastError()
  defer { clearLastError() }
  let result = call()
  if let error = error.error {
    throw error
  }
  return result
}

Usage

Property get: let foo = try withFailingCall(obj.foo)
Property set: try withFailingCall(obj.foo = "invalid")

Changes

  1. _tryWinRT methods were added for generated code to invoke. These methods capture the file and line of the caller and add that to the exception information so you know which line failed.
  2. an error message is provided when one of these errors is unhandled, telling the developer of the possible workaround:

An example of the message:

test_component/ERROR.swift:263: Fatal error: Unhandled WinRT Error @ test_component/test_component.swift: 1098 - 0x80004001 - Not implemented

  Note: If this error is expected and needs to be handled, use `withFailingCall`

@stevenbrix stevenbrix requested a review from a team as a code owner March 4, 2024 20:37
Copy link
Contributor

@tristanlabelle tristanlabelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very clever way to go about the problem but I am concerned about the overhead of this solution. I think the original let foo = try obj._default.get_Foo() was an okay solution (if non-default interfaces are also exposed). There is also an alternative way to solve this: generate get_Foo() (or _get_Foo) on the object directly and have the var foo accessors call it with try!.

@stevenbrix
Copy link
Collaborator Author

stevenbrix commented Mar 5, 2024

This is a very clever way to go about the problem but I am concerned about the overhead of this solution. I think the original let foo = try obj._default.get_Foo() was an okay solution (if non-default interfaces are also exposed). There is also an alternative way to solve this: generate get_Foo() (or _get_Foo) on the object directly and have the var foo accessors call it with try!.

what overhead are you referring to? i don't think it's all that bad. the only overhead is:

  1. when the apps is about to fatalError and die
  2. someone uses this API to handle an error.

We've written arc so far and there is one API call in the entire app that we'll need this for and that's because we support running packaged and unpackaged. this is truly an abnormal circumstance that someone needs to use this API.

agreed we could also have @_spi(Internal) func _getFoo()/_setFoo() methods but that still bloats the export count


fileprivate var getErrorLock = NSLock()
fileprivate var checkErrorLock = NSLock()
fileprivate var lastError: LastErrorHolder?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need to be thread-local. Concurrent calls should have independent last errors to not step on each other's toes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thread local would prevent the need for locks, but the last error is only used in the scope of the withFailingCall method, so the other thread will block waiting to get access to the last error. at the end of withFailingCall the error is cleared

@tristanlabelle
Copy link
Contributor

I'm not comfortable with the approach of using a global to pass information from the callee to the caller. I think this will break down in more complex scenarios like try withFailingCall(obj.foo = getFoo()), which would make the whole body of getFoo() execute in non-fatal mode and return dummy return values on failure.

For overhead, there's an autoclosure and a try/catch that I'm also concerned about, especially if the the projection is built as a dll.

Copy link
Contributor

@tristanlabelle tristanlabelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking based on last comment. I think this approach will misbehave when the autoclosure ends up capturing multiple WinRT calls, which can happen in benign-looking code.

public func withFailingCall<Result>(_ call: @autoclosure () -> Result) throws -> Result {
let error = getLastError()
defer { clearLastError() }
let result = call()
Copy link
Contributor

@tristanlabelle tristanlabelle Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break down in scenarios like try withFailingCall(obj.foo = getFoo()), which would make the whole body of getFoo() execute in non-fatal mode and return dummy return values on failure.

@jeffdav jeffdav removed their request for review March 15, 2024 16:04
@stevenbrix stevenbrix closed this Sep 16, 2024
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

Successfully merging this pull request may close these issues.

2 participants