Separate internal/recoverable errors from the public LoadingError/RenderingError
After #597 (closed), there are some things I think we should address with respect to error handling:
-
The public LoadingError/RenderingError should be about errors from which librsvg cannot recover, and should bubble up to the caller as fast as possible. Instead, right now some of the inner code generates such an error, sometimes catches it, and recovers from it. For example, image.rs calls
acquired_nodes.lookup_image(url)
and, per SVG's customary resiliency, just doesn't render the<image>
if the resource couldn't be read from disk. Digging intolookup_image
we have thatDocument.images
is aHashMap<AllowedUrl, Result<SharedImageSurface, LoadingError>>
. We should use a finer-grained error there, ideally one that lets us distinguish "I/O error" from "codec says image is corrupted". I think the filters code does something similar where it caches the filter background as aResult
. -
Internally we should have errors with full information - store a glib::Error for I/O errors from io.rs, or maybe a rustified version of
xmlError
for XML parsing errors - but we should not expose those implementation details in the librsvg Rust API. #597 (closed) did this by havingString
details in the public error variants, but we should useBox<dyn Error + whatever>
for the variants instead. Librsvg is going to be changing the XML parser and probably the image loader in the future, and we should avoid an API break that would happen if we exposed the internal, backend-specific errors to the public API. Which leads me to... -
... we can't immediately use
Box<dyn Error + ...>
right now, because the first point above requires that error types implementClone
, because they are stored long-term (and we can'tBox<dyn Error + Clone>
). If we move to finer-grained errors for "internals", I think we'll be able to move to boxed type-erased errors for the public API. -
I haven't fully investigated if the error crates like
thiserror
would help. They may help in shrinking the code used to construct internal errors, which is fine, but AFAIK they don't help with exposing type-erased errors to the public API.
Side note: https://sled.rs/errors.html is really interesting and I encourage everyone to give it a read. I don't know if librsvg should adopt its scheme of Result<Result<Whatever, RecoverableError>, FatalError>
, but it's something worth thinking about. #547 (closed) would not have happened with that in place; maybe it's overkill to do this double Result everywhere...