Convert internal X11 errors to anyhow. #982

jneem · 2020-05-23T04:15:44Z

This isn't really a serious PR, but I wanted to discuss something along these lines. Trying to add present support for X11 Present, I had lots of RefCell borrowing and, as a result, hundreds of lines of error handling boilerplate. This is an attempt to reduce that.

I'm not sure that adding a new dependency is the way to go, but I do think we want more ergonomic "generic" errors in druid-shell, particularly in the platform code. There are so many different errors that can come from the system, where it doesn't make sense to do much more than print a backtrace. Since many of these errors can never actually make it back to druid, it doesn't make much sense to have nice enums for them; that's why I thought of giving anyhow a try.

xStrom

Overall this does look like a good way to reduce boilerplate so I think it's worth trying out. The error story of druid-shell is rather weak/new. This X11 stuff I wrote just a few weeks ago, and some of it I already reconsidered when working on some macOS changes. So the current state certainly isn't great.

Let's try out anyhow and see how it goes. We can polish up this PR and have it merged.

druid-shell/Cargo.toml

druid-shell/src/platform/x11/window.rs

xStrom

Looking better! I pointed out some remaining issues. Also please add a changelog entry under maintenance.

druid-shell/src/error.rs

xStrom · 2020-05-24T11:30:58Z

druid-shell/src/platform/x11/application.rs

@@ -14,6 +14,7 @@

 //! X11 implementation of features at the application scope.

+use anyhow::{anyhow, Context, Error};


Move this import lower next to xcb.
The general grouping order of imports should be: std / external / crate / super / descendants.

This is still there too.

xStrom · 2020-05-24T11:40:31Z

druid-shell/src/platform/x11/mod.rs

 pub mod keycodes;
 pub mod menu;
 pub mod window;

+// TODO: This is just a placeholder, because we haven't removed all the other platform errors yet.


I'm not sure that removing platform errors is the way forward. If you look at the Windows platform error code for example, it contains some rather useful HRESULT handling.

Using anyhow instead of Generic/Other is fine. Using the new macros instead of BorrowError is reasonable. However I think there's still room for platform specific errors that are specifically designed, like the Windows Error::Hr(HRESULT).

I'm open to hearing a good argument of course, but otherwise I think it makes sense to still have some X11 platform Error type, even if really minimal.

I could go either way on this, but let me explain my rationale anyway:

The only point (AFAICT) of having an error enum is to allow users to match on it, and I have trouble seeing a use-case for matching on a platform-specific error from druid-shell.

One reason for this is that druid-shell actually handles most of its own errors, because it's in charge of the run loop. I had a quick look through the API, and I only saw Application::new and WindowBuilder::build as places that the user gets an error.

The platform-specific nature of a platform error also makes me think that it's unlikely to be handled by the user, together with the fact that -- looking at the current set of errors -- I don't see what they would do with one besides logging it and exiting.

Having said that, anyhow::Error does allow the user to get the underlying error if they really want it, because it supports downcasting. The main disadvantage compared to an error enum is ergonomics and discoverability.

Not a lot of errors being surfaced outside of druid-shell is more about it just not having been done yet. However I agree with your point that it's unlikely anyone will want to match against platform specific errors.

However the platform code itself might want to match against its own errors. Error handling isn't always going to be exactly where it happens.

Also the Windows Error::Hr(HRESULT) continues to be very useful. There's no base error that can just be contexted. Thus I still don't see the platform specific Error type going away.

Fair enough; I've reinstated the X11 error module.

druid-shell/src/platform/x11/application.rs

druid-shell/src/platform/x11/window.rs

xStrom · 2020-05-24T19:17:58Z

druid-shell/src/platform/x11/error.rs

    }
 }
-
-impl std::error::Error for Error {}


Can we keep this impl line as well?

jneem · 2020-05-25T03:44:21Z

Sorry, I think I made a mistake with git stash. These should be fixed now.

xStrom

Almost there!

Unfortunately the nightly compiler is breaking builds so we can't merge this yet. I opened #985 to address this, which is now merged. So you can rebase with master to get the CI to pass.

xStrom · 2020-05-25T08:38:11Z

CHANGELOG.md

@@ -143,6 +143,7 @@ This means that druid no longer requires cairo on macOS and uses Core Graphics i
 - Refactored DPI scaling. ([#904] by [@xStrom])
 - Added docs generation testing for all features. ([#942] by [@xStrom])
 - Renamed `BaseState` to `WidgetState` ([#969] by [@cmyr])
+- X11: Reworked error handling ([#982] by [@jneem])


Need to also add the link to the bottom of the file.

luleyleo

I only skimmed this, due to a lack of time, but it looks like a nice change.

luleyleo · 2020-05-25T09:10:22Z

druid-shell/src/platform/x11/window.rs

+            match borrow_mut!(self.handler) {
+                Ok(mut handler) => {
+                    anim = handler.paint(&mut piet_ctx, invalid_rect);
+                    err = piet_ctx
+                        .finish()
+                        .map_err(|e| anyhow!("Window::render - piet finish failed: {}", e));
+                }
+                Err(e) => {
+                    err = Err(e);
+                    let _ = piet_ctx.finish();
+                }
+            };


The second finish call could fail as well, right? If we can add it to err, than that would seem useful, otherwise we could just keep loging this one.

The only way I can find to return a double error is context, which isn't really appropriate IMO because it's for when one error is caused by another. I added a log, though.

xStrom

Thanks for your effort!

cmyr · 2020-05-26T14:32:27Z

I think this is an improvement but I don't think adding a dependency is the best route long-term; in general I think that crates like anyhow make sense for applications, but less so for frameworks, where they can force downstream crates into a particular error handling path.

There's some discussion of a longer-term error strategy somewhere; but basically I think that each platform should have an internal error type that closely matches whatever the native error type on that platform is, and then druid-shell should have a set of errors for common situations and then should also wrap the platform error type for more detail.

xStrom · 2020-05-26T15:06:46Z

I understand the usual dependency issues like compile time, quality issues etc. However when directly talking about the error types, I'm not sure I follow how anyhow will force any specific path.

What difference does it make if Error::DruidShellOpaqueVariant is Arc<anyhow::Error> or CustomReimplementationOfAnyhow? In both cases it implements the Error trait and you can print it, wrap it, clone it etc. In neither case can you really get anything more out of it than a string describing the error.

jneem · 2020-05-26T16:05:01Z

Just to elaborate on what @xStrom already said, I agree that we should wrap the native error type (and #989 introduces this for X11), but there are also plenty of internal druid-shell errors for which I don't think we should commit to having a variant in the public API.

cmyr · 2020-05-26T16:21:04Z

/bloat

(curious to know if this works after merge?)

To be fair I haven't read through this carefully, just trying to keep my head above water. I'm certainly happy for any improvements to error stuff in the short term.

jneem · 2020-05-26T16:40:00Z

It doesn't seem to be doing much...

Isn't bloat only for mac, though? Given that I didn't touch the mac backend, I wouldn't be surprised if whatever bloat I introduced just gets optimized out...

cmyr · 2020-05-26T16:41:37Z

yea apparently it won't work if something has been merged. And fair point, that will need reworking at some point. :)

xStrom reviewed May 23, 2020

View reviewed changes

xStrom added S-waiting-on-author waits for changes from the submitter shell/x11 concerns the X11 backend labels May 23, 2020

xStrom reviewed May 24, 2020

View reviewed changes

druid-shell/src/platform/x11/error.rs Outdated

}

}

impl std::error::Error for Error {}

Copy link

Member

xStrom May 24, 2020

Choose a reason for hiding this comment

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

Can we keep this impl line as well?

xStrom reviewed May 25, 2020

View reviewed changes

luleyleo reviewed May 25, 2020

View reviewed changes

jneem added 10 commits May 25, 2020 11:53

Convert internal X11 errors to anyhow.

61468de

Simplify error types some more, and introduce macros for borrow errors.

bd5f7c4

Put anyhow in the right place.

d12cf59

A bit of missing context.

3f21b43

Simplify control flow.

30fdbf6

Address most review comments.

cf278ae

Resurrect the X11 error module.

9f9b309

I was sure I fixed these...

935ecd7

Add changelog link.

0585776

Log the double error.

213b5ab

xStrom approved these changes May 25, 2020

View reviewed changes

xStrom merged commit 559c534 into linebender:master May 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert internal X11 errors to anyhow. #982

Convert internal X11 errors to anyhow. #982

jneem commented May 23, 2020

xStrom left a comment

xStrom left a comment

xStrom May 24, 2020

xStrom May 24, 2020

xStrom May 24, 2020

jneem May 24, 2020

xStrom May 24, 2020

jneem May 24, 2020

xStrom May 24, 2020

jneem commented May 25, 2020

xStrom left a comment •

edited

Loading

xStrom May 25, 2020

luleyleo left a comment

luleyleo May 25, 2020

jneem May 25, 2020

xStrom left a comment

cmyr commented May 26, 2020

xStrom commented May 26, 2020

jneem commented May 26, 2020

cmyr commented May 26, 2020

jneem commented May 26, 2020

cmyr commented May 26, 2020

		@@ -14,6 +14,7 @@

		//! X11 implementation of features at the application scope.

		use anyhow::{anyhow, Context, Error};

Convert internal X11 errors to anyhow. #982

Convert internal X11 errors to anyhow. #982

Conversation

jneem commented May 23, 2020

xStrom left a comment

Choose a reason for hiding this comment

xStrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneem commented May 25, 2020

xStrom left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luleyleo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xStrom left a comment

Choose a reason for hiding this comment

cmyr commented May 26, 2020

xStrom commented May 26, 2020

jneem commented May 26, 2020

cmyr commented May 26, 2020

jneem commented May 26, 2020

cmyr commented May 26, 2020

xStrom left a comment •

edited

Loading