On Thu, 03 May 2007 14:11:02 -0400, Havoc Pennington wrote: > Yeah, I think this keeps the Cairo model from being "fully general" - it > only works for some types of operations or objects. Sure. If you want the object to remain usable after an error, then it shouldn't shut down. And note that that with cairo, we do have functions that do return an error indication instead of shutting down, (cairo_surface_write_to_png, for example). So I think a fair characterization of the "cairo model" is that it recognizes that you don't always do the shutdown-on-error thing, but it is darn handy when it's the right thing to do. > Another thing about > the Cairo model is that it's either weird or possibly broken if an > object is used by multiple threads, which could arise in the current > gvfs presumably. For cairo, at least, there's nothing broken here. If you want to use the same object from multiple threads, then you need to be doing your own locking. As for weird, the shutdown thing should only be happening if there's really no other appropriate response. There's a difference between causing an error _in_ an object, (in which case it will shutdown, and it's appropriate for it to be shutdown for all threads), as opposed to causing an error _with_ an object, (in which case it should just return the error indication and not shutdown). > Another thing that's needed for gvfs for sure but not for Cairo perhaps > is the error message. The model of lodging errors inside the objects doesn't prevent you from putting as detailed a message in their as you'd like. With cairo itself, we don't actually store anything more than an enum, but... > 8859-1"), but nonetheless in a lot of cases I think a detailed error can > be better than a strerror-style string. Sometimes the error will be > user-helpful, e.g. "You aren't allowed to save to directory Foo" ... even with the very strerror-style approach that cairo encourages, it's very easy for the calling application to add context like this when generating its error message. > Returning *helpful* errors, even if only to programmers and techies, > also frequently depends on context - meaning, the error can be more > helpful if it's checked right away after every call. Sure. So everytime you want to emit a useful error message, check the status and generate the message with all the context you want. Or, like I said above, you could envision applying the cairo model and also adding a string to it. (That is, if applying the cairo model pushes the GError from a parameter to every function to instead be a field within the object, that doesn't mean that GErrror needs to become any less capable). > Finally, Cairo of course mixes programmer errors (g_return_if_fail in > GLib-world) with should-be-reported errors (GError in GLib-world) with > unavoidable-and-should-be-ignored errors (just get ignored in > GLib-world). See GError docs for how these are separated in GLib: > http://developer.gnome.org/doc/API/2.2/glib/glib-Error-Reporting.html I definitely agree with you that there are different classes of errors and they require different kinds of handling. And I won't argue that cairo gets all of this right yet. (In a very real sense I've always felt that cairo's error-handling strategy was a big experiment, and it would be interesting to see how it played out. So I'm quite glad to see this discussion happening around it.) In the meantime, it's not clear to me that either cairo or glib has this all figured out yet. For example, the document above says: First and foremost: GError should only be used to report recoverable runtime errors, never to report programming errors. If the programmer has screwed up, then you should use g_warning(), g_return_if_fail(), g_assert(), g_error(), or some similar facility. For that second sentence, if we're talking about a non-recoverable programmer error, then what guidance is there for choosing g_return_if_fail as opposed to g_assert? Are programmer errors actually divided into two classes? I could imagine a consistent argument that all programmer errors should lead to an abort, (to force errors to get noticed and dealt with early). And I could accept that some people would want to configure the abort away as well, (which G_DISABLE_ASSERT allows for example). So I could imagine a consistent argument that g_assert should be used for all detected programmer errors. But how does g_return_if_fail fit into your model? Isn't it really doing basically the same thing as cairo's inert object functions? Differences I see are that: 1. g_return_if_fail prints a message 2. cairo's inert objects come with a guarantee that once an object shuts down, no future call to that object will have any effect. > For me when using Cairo drawing I would say should-be-reported errors > aren't really possible. When the problem is that I passed nonsense to a > Cairo API, I would prefer some verbose warning to the silent failure > plus an error code I have to figure out the origin of. So all you really want is a mode to make cairo be noisy on the first detected error? That would be a simple change to the _cairo_error function through which all of the shutdown-an-object errors should be passing. People have mentioned wanting it before, but I haven't cared about it enough to add it myself. I do feel that cairo should not print anything unless it's about to abort, or unless the user has explicitly requested it by setting some environment variable or something. But a little noise isn't really enough information to tell you where the error came from, so you're still going to have to track it down. What you'd really like is a nice application-level stack trace from when the error gets detected by cairo, (hopefully people using bindings are getting this already). Me, I just use gdb, set a breakpoint on _cairo_error and trigger the problem to find the problem. > When the problem > is something unavoidable (say not enough memory or whatever) then I just > don't care, I'm not going to check the error code for this or in any way > recover. So in neither case here do I find the error codes useful. And here's the other big disagrement between you and me. You're free to write applications that don't care about out of memory, (most of mine don't---but I only write toys). But that's definitely not a decision I'm willing to force on to all users of a system library like cairo. So the fact that some users want to be able to recover from this and some don't care is definitely a case where cairo's error-handling strategy really helps. Note that both users can still benefit from having the warn_unused_result attribute turned on for every cairo function that returns a value. (We actually had a paatch committed during 1.4.x that added that attribute, but I've deferred it until 1.6). > When the programmer _should_ check the error, e.g. > cairo_surface_write_to_png, I think GError-style reporting is ideal, and > the return code as write_to_png has is OK and does the job and is pretty > much equivalent to GError except there's no memory management (plus) and > no error message (minus). I'll agree that the memory management of GError is a huge minus. If it makes me old-fashioned to care about providing an interface that's easy to use from C, so be it. > - cairo model is most usable when it can be correct to ignore errors, > which is particularly common for programming errors > (GLib would use return_if_fail/assert) or unavoidable runtime errors > that are best handled by simply ignoring them Yes, that's when shutdown-the-object works best. > - cairo model should perhaps be extended to support an error message > for more general applications (since in the drawing case the idea > is to almost always ignore errors, there's no point, but for file > and network ops, there may well be) Sure. Shove a string in there if you want it. > - cairo model raises issues when the error state is shared by > multiple threads accessing one object Looks like a non-issue to me. If the object is only getting shutdown due to an error you want to ignore, then this shouldn't be a problem. > - when it's really probably wrong to not check errors (e.g. when > saving a file), a GError-type model with a function arg you must > consider, or a return value with warn_unused_result as Carl mentions, > is better than the cairo_t store-error-on-object model IMO Yes. And the cairo library explicitly doesn't use store-error-on-object for thiese cases. > - I personally believe programmer errors should get the return_if_fail > or assert type treatment and not be runtime-detectable, because > a nice warning is more helpful to the programmer or in bug reports, > and runtime detecting these is just wrong anyway - fix the bug, don't > recover from the bug by adding bug-handling code I agree that programmers should fix their own bugs rather than writing new code to recover from the bug. I don't see anything in the cairo model that encourages writing bug-handling code. > It's very very important IMO to distinguish the three types of errors > (programming, recoverable, and unavoidable-and-ignorable) when designing > any API in this area. I agree it's important to distinguish. And for the three cases: Programming errors Glib seems inconsistent about these, (g_assert provides a reliable way to handle them, but sometimes g_return_if_fail is used instead which is inherently unreliable). Cairo tries to shutdown the affected object whenever possible, (choosing to just segfault instead if the user didn't even pass a valid object in the first place). But not that the "invalid object" is something that can't even arise from cairo itself, (which _is_ actually a benefit of the never-return-NULL-from-constructors approach). Recoverable Glib and cairo are similar if the application author is checking the status after every call, (but for the memory management and error message points noted above). Cairo does allow for the error-checking to be deferred if desired. Unavoidable-and-ignorable Glib considers this category to exist, and aborts. Cairo does not admit the existence of this category. Instead cairo uses the same shutdown strategy as is used for programming errors. This allows recovery to be handled in the most convenient way possible by the application, (again, taking advantage of deferred error checks). So that looks to me like if you modified _cairo_error to do the equivalent of g_assert, you'd basically get what you want for your recoverable and unavoidable cases. But that would leave you with always having g_assert and never g_return_if_fail for programming errors though. So please explain more to me about how that handling should be distinguished. -Carl
Attachment:
pgpEV2sx4hG9L.pgp
Description: PGP signature