Making it harder to swallow `error.Canceled`

Brainstorming
42

I forget, since it’s now been two release cycles since Writergate, but if you were interested in why, you could check out the release notes for 0.15.0 or @andrewrk’s excellent talk entitled “Don’t Forget To Flush” from that era.

43

A possible rule of thumb:

Errors are about control flow, and providing a useful information for the caller to be able to take a useful action. So, rename WriteFailed to CheckWriter. Then, don’t ever return WriteFailed from a function (Io.Writer obviously exempt). Instead:

  1. If you own the writer, check the writer’s impl for the error and return that (or some other more specific error, or take some other correct action, just never return the CheckWriter).
  2. If you don’t own the writer (you were passed an Io.Writer), always catch CheckWriter and replace it with an error more specific to the writer which produced the error. eg, if you took an argument foo: *io.Writer, you would turn error.CheckWriter to error.CheckFoo.

(Also true for Reader, just didn’t want to write Reader/Writer a bunch)

This bug (and it is a bug), for example, appears to be because multiple readers are layered on top of each other, and one of those layers (I think specifically this line: https://codeberg.org/ziglang/zig/src/commit/8901bfe190e2360f934c6e4330cac5eed6c8712b/lib/std/http.zig#L546) returns a reader error which can be incorrectly “smuggled” as an error of the http level reader. As such, std.http.Reader.BodyError should have errors specific for checking the underlying transport reader/writers. So bodyErr would be correctly set to something like error.CheckConnectionStreamReader.

7 Likes
44

I know these.

I just think that the listed motivations are less bad than the downsides coming from this situation here.

1 Like
45

All of this is good theory.

But after using it (and with the feedback from others here) imo impractical because you forget it too easily, especially when you need to look up which Reader/Writer in a chain failed.

The goal behind a Reader/Writer interface is to create something which is generic over multiple implementations.
And that’s hard because there are multiple core strategies for this:

  • compile time monomorphization
    This is the pre-0.15 interface
  • runtime polymorphism
    Here you have for error reporting two possibilities again:
    • return any possible error
      This would essentially be using anyerror in Zig.
    • doing it out of bounds
      You put the actual error in some special place (here a struct member, in libc normally errno), tell somebody where that is and return a marker so that the caller can check for it (manually or automatically; the latter being a possible strategy for implementing exceptions).
      Or you have a secondary return path for errors (another possible strategy for implementing exceptions and the one taken by C++ I think).
      This is the post-0.15 interface.

The problem of the standard library here is that it has to chose one of these possibilities to “bless” for the ecosystem of the language to build around.
But every choice trade some things for others and the correct choice depends on the application and your priorities, which will always differ between people.
Here some tradeoff examples (there are more but that would be way too much to write out):

  • Monomorphization gives up an ergonomic API boundary and binary size in favour of precision and less indirection.
  • Returning any possible error gives up precision in favour of simplicity.
  • Returning the error out-of-bounds gives up ease of use in favour of uniformity.

There is no free lunch. You will end up with something which is ugly in one aspect but great in another. You will also always end up with something where some people will say that you took the worse option because everyone ranks all of this differently.

So what to do about error.Canceled?
It depends.
I would make it part of the error set of the Reader/Writer error set.
Sure, you can say that this doesn’t make sense for every implementation, but error.ReadFailed and error.WriteFailed also don’t make sense for every implementation too (see std.Io.Reader.fixed and std.Io.Writer.Discarding as examples).

5 Likes
46

Fixed should very clearly be able to trigger ReadFailed, of course. I appreciate you summing up the tradeoffs of the three options! I’m happy to agree to disagree with your assessment, the third option seems best to me still.

47

I’d like to give a challenge to anyone who claims that having the out-of-bounds errors is the best option. Please, take std.http and refactor it to avoid cancellation problems structurally. I really mean it, I’d like to see some example strategy to follow.

7 Likes
48

Can you elaborate?

2 Likes
49

don’t threaten me with a good time :wink:

50

How should it?
You can get an error.EndOfStream, but that’s it.

1 Like
51

I’m not following how that experience, specifically the last part, follows from my suggestion. You shouldn’t ever have ambiguity about which reader/writer has an error. (Aside from using std which doesn’t follow this pattern, so also isn’t a good test of the pattern.). If you encounter a Reader/WriterFailed where you don’t know the source, that’s a sign you need to go to the places which are generating the error and either:

  • change it to an error indicated where to go for the proper error
  • or if the source of the error is available there, unwrap it and handle that error as you would if it were returned from the Writer/Reader directly.
52

One thing that I realized people might not be aware of is that you can defensively program against errors being allowed at callsites:

test {
    foo() catch |err| switch (err) {
        error.WriteFailed => comptime unreachable,
        error.ReadFailed => comptime unreachable,
        error.Canceled => comptime unreachable,
        else => |e| return e,
    };
}

fn foo() !void {
    // ...
}

This compiles. However if you return error.Canceled for example from foo, then you get a compile error:

test.zig:5:36: error: reached unreachable code
        error.Canceled => comptime unreachable,
                                   ^~~~~~~~~~~
4 Likes
53

With a Reader/Writer chain I mean a Reader/Writer which takes a Reader/Writer as a source.

For example let’s say you have a gzip reader. It takes a gzip source and gives you the uncompressed data when reading from it.
But where does the source come from? Well, how about another Io.Reader? That way it can be a slice (via fixed), a file, a socket or something else.
But obviously the Reader can fail (for example if the data is bad), but also reading from the source can fail (e.g. if the source for the gzip reader is a file).
So if you use the Io.Reader of the gzip reader, you just get an error.ReadFailed. But which one had the actual error? The gzip reader or the other one? Well, now you need to look that up.
And that process is imo highly error prone.

6 Likes
54

I didn’t even know that you can have unnamed tests.

But I would probably do this via metaprogramming instead, because normally functions, where you maybe don’t want to return an error.ReadFailed etc., have the kind of input, which isn’t exactly easy to make up for such a test.

So probably this:

fn disallowErrors(Function: type, DisallowedErrors: type) error{DisallowedError}!void {
    const ReturnType = @typeInfo(Function).@"fn".return_type.?;
    const ActualErrorSet = @typeInfo(ReturnType).error_union.error_set;
    // the following two lines are for 0.16 and different for master
    const actual_errors = @typeInfo(ActualErrorSet).error_set.?;
    const disallowed_errors = @typeInfo(DisallowedErrors).error_set.?;
    for (actual_errors) |actual| {
        for (disallowed_errors) |disallowed| {
            // drop the `.name` for master
            if (std.mem.eql(u8, actual.name, disallowed.name)) {
                std.log.err("encountered disallowed error: {s}", .{actual.name});
                return error.DisallowedError;
            }
        }
    }
}

test {
    try disallowErrors(@TypeOf(foo), error{WriteFailed});
    try disallowErrors(@TypeOf(bar), error{WriteFailed});
}

fn foo() !void {
}

fn bar() error{WriteFailed}!void {
}
1 Like
55

Right, so. My suggestion means in this scenario is that if gzip encounters an error, it sets its internal error to something like error.CheckGzipInput, and then the reader returns error.CheckReader. So usage code would look something like this:

fn main(init: std.process.Init) void {
    const io = init.io;
    
    var file: std.Io.File = std.Io.Dir.cwd().openFile(io, "myfile.txt", .{}) catch |err| switch (err) {
        // file open errors...
    };
    defer file.close();
    var file_buffer: [1024]u8 = undefined;
    var file_reader  = file.reader(io, &file_buffer);

    var gzip_buffer: [1024]u8 = undefined;
    var gzip_reader: std.compress.flate.Decompress = .init(&file_reader.interface, .gzip, &gzip_buffer);

    const ast = parse(&gzip_reader.reader) catch |err| switch (err) {
        error.InvalidAst => {},//.....
        error.CheckParseReader => switch (gzip_reader.err.?) {
            error.CheckGzipInput => switch (file_reader.err.?) {
                error.Canceled => // ...
                // and other file errors
            },
            error.BadGzipHeader => // and other decompression errors, etc..
        },
    };
    _ = ast; // ...
}

const ParseError = error{InvalidAst, CheckParseReader};
fn parse(reader: *Io.Reader) ParseError!Ast {
 // ...
}

That is, the error.CheckReader (error.ReadFailed ) tells you to unwrap the outermost layer, which may itself tell you to unwrap the next layer down.

Fwiw, it looks like std.compress.flate basically does this already. It just doesn’t do my suggested extra error renaming, to reduce ambiguity and the probability of bugs that, eg, were hightlighted in http earlier in the thread.

56

Three problems with this:

  1. You are dealing with the simple cases I called “tolerable”, because you are creating the readers in a single function and handling the errors directly in there as well. Now imagine the chain is dynamic, depends on some runtime parameters. Maybe you are reading config file, and sometimes you need to insert the gzip reader, sometimes you don’t. Now you need to evaluate all the same runtime parameters, to determine which readers are actually in the chain.

  2. You are assuming foo_reader.err is not null. That’s a bold assumption. Have a look at discardDirect in the flate decompressor. If it gets cancelled during drain, you err is not set. In reality, you always need to assume the value can be null.

  3. And now the problem that was mentioned earlier in the thread. When std.Io.File.Reader determines the reading mode, the error can end up in one of file_reader.err, or file_reader.size_err, of file_reader.seek_err and you need to check all, because you don’t know which one is it.

So it’s not just renaming, even in the error out of bounds solution, this needs a larger design change.

10 Likes
57

Behind this is a problem with error handling design, namely that we need an additional system to record specific error messages beyond just the error codes.

I still think the final solution might need a dependency-injected error reporter, where the error codes indicate which error reporting method to use, and the reporter can be decided by the caller to ignore/log/read the reported error info from a specified memory location.

58

I’m trying to fix error handling in tls.zig and I’d like your opinion. What do you think is the best way to indicate transport error, to be stored in conn.reader.err, using the current conventions?

  1. null (clear)
  2. error.ReadFailed (forward)
  3. error.TransportReadFailed (rename)

I’m currently of the opinion that the first option leads to the cleanest code on both sides, but maybe the third option wins because it’s the most explicit, just the code gets awkward, because the null state then basically defines illegal state.

59

Imo an error field or a reader or writer being null should mean that there was no error, neither on itself or a layer down (if it is a reader/writer wrapper).

7 Likes