search

contextfree / winrt-rust

Use and (eventually) make Windows Runtime APIs with Rust
Apache License 2.0
142 stars 10 forks source link

Reconsider error handling #67

Closed Boddlnagg closed 5 years ago

Boddlnagg commented 5 years ago

We currently return Result for every wrapped method, by inspecting the HRESULT that is returned from the wrapped call, which is the channel through which exceptions are returned in WinRT.

However, according to https://blogs.msdn.microsoft.com/oldnewthing/20190228-00/?p=101055 there's guidance that "Application code is not expected to catch exceptions thrown by Windows Runtime objects because they represent unrecoverable errors".

This means that we could also panic! in those cases, which would simplify the API.

However, some API methods don't respect that guidance (according to the same blogpost the reason is that they precede this guidance). New methods are being introduced with explicit error handling, but they do not yet exist everywhere.

BenJKuhn commented 5 years ago

I'll second what Raymond has said. Although the guidance is that Windows Runtime errors are unrecoverable errors, what constitutes unrecoverable is different in different contexts. Most errors from Windows Runtime APIs simply indicate that the operation failed and cannot be continued or recovered, but that the process is in a stable state. If the code in the API didn't already halt the process and instead returned a failure, there's no heap corruption or stack corruption.

As a simple example, many document editors will observe and handle failures from the printing stack, prompt the user about the failure, but still continue to allow the user to continue working with the application. The printing operation didn't complete, and printing again to the same device with the same settings will likely yield the same result (so in that sense there's nothing the application can do to mitigate the situation), but there's little concern that the document state was corrupted. Further editing or saving the document is typically still safe.

But not always. Failures in an operation can expose robustness issues in other parts of the application. For instance, if the application locked a resource on start, but didn't unlock the resource due to the failure, that could cause further instability or deadlocks later during operation of the application.

The short of it is that panic! as a universal solution is likely too aggressive to be practical, even though that will be a common way to handle many such errors.

Ben (Microsoft)

Boddlnagg commented 5 years ago

Then you are right that "what constitutes unrecoverable is different in different contexts", and what it means for WinRT is not the same as what it means for Rust (https://doc.rust-lang.org/std/macro.panic.html):

panic! should be used when a program reaches an unrecoverable problem.

I think I agree with you that using panic! here would be too aggressive, but I'm still going to leave this issue open in case some more people want to chime in.

liamdawson commented 5 years ago

Panicking from a library has negative consequences on consuming code, so I'd ask it only happens if the application state is now tainted. That's probably not the case if I try to create a desktop notification and it fails for some reason.

Boddlnagg commented 5 years ago

Okay, I think it's clear that we shouldn't use panic! for failures indicated by HRESULT. I'm going to close this.