Making it harder to swallow `error.Canceled`

Brainstorming
1

This is related to my other topic, pretty much every single library I look at swallows error.Canceled via the reader/writer interfaces. It’s understandable, because the error.ReadFailed → reader.err orelse err is not obvious. I brought it up on Zullip a long time ago, but Andrew was against adding error.Canceled to the reader/writer interfaces. I still think it should be added there, otherwise we will only continue to have more and more libraries that make tasks uncancelable. However, in general, even the stdlib is not immune to this, for example I have one open PR to fix swallowing of error.Canceled in std.Io.Condition. And there were multiple PRs to fix swallowing of the error in std.debug and std.log. So two questions:

  1. Could/should error.Canceled be added to std.Io.Reader/Writer errors?

  2. Could the language add some feature that makes swallowing error.Canceled hard?

3 Likes
2

Your solution helps Canceled get handled correctly more often, but it doesn’t help other error types while adding a Canceled error to the set in places where Canceled is impossible.

If this is a common bug, I think the fault lies in the reader/writers’ error. Perhaps error.ReadFailed → error.CheckReader or something similar?

3 Likes
3

What about having to tell Io that you acknowledged a cancellation, then a DebugIo could complain when there are outstanding cancellations at various points?

This would, however, be less ergonomic than a language solution

4

The problem is deeper, there are good use cases for passing *std.Io.Reader to a function, but such function can’t recover the error anymore. The calling function would have to assume that if it got error.CheckReader, it means it’s own reader, even though the error could come from some internal reader in the function. I guess you could depend on reader.err never being overwritten, but I find it a sketchy design. Plus, people just use try, they don’t even list the errors. I don’t blame them, it gets tiring trying to uncover error.Canceled from every single read call, it’s even worse if you do multiple writes + flush.

5 Likes
5

Particular errors in the error set being impossible is already relatively common and something Zig programmers are expected to deal with in many circumstances. For example, writing to std.Io.Reader.fixed when the buffer is large enough can’t actually return error.ReadFailed. In an infallible function you’d just catch unreachable on all of the reader’s function calls.

Personally, I think that makes the most sense. I think that enough implementations would handle a true Reader/Writer error differently than an error.Canceled that it makes sense to separate their handling at the top level. For example, reporting an error to the user on error.ReadFailed vs just returning on error.Canceled.

Alternatively, the “detailed diagnostics” talked about in std.Io.Reader and std.Io.Writer error set definitions should be promoted to the interface so that it’s available to code actually using the Reader/Writer.

2 Likes
6

Even ziglings teaches these bad patterns :frowning:

https://codeberg.org/ziglings/exercises/src/branch/main/exercises/091_async7.zig#L57

I could see some justification for catch return, even though I’d also avoid that in educational material, but catch {} just shows how misunderstood cancellation in std.Io is. For people not familiar with it, if you do things like the mentioned io.sleep(...) catch {};, your task becomes uncancellable, no other operation will return error.Canceled, the error is only delivered once to allow clean shutdown, but swallowing it means you make the task uninterruptible.

5 Likes
7

Another example, from the “Introduction to Zig” book:

By not retrieving error.Canceled from writer.err, calling code might mistake this for an error that can be just logged and ignored.

Not to mention that write is not the right function to use there, it might short-write, this should have used writeAll. I guess what I’m finding that we are teaching new users all the bad patterns.

6 Likes
8

Why whould you do it? Separate the interaction with the interface and the intercation with the implementation.

fn readFromFile(io: std.Io) !Whatever {
  var buffer: [1024]u8 = undefined;
  var stdin = std.Io.File.stdin().readerStreaming(io, &buffer);
  const whaterver = readFromInterface(&stdin.inteface) catch |e| switch (e) {
    error.ReadFailed => return stdin.err.?,
    else => return e,
  }
  return whatever;
}

fn readFromInterface(reader: *std.io.Reader) !Whatever {
  var whatever: Whatever = .{};
  // do not need to do anything with error.Canceled here
  whatever.foo = parseFoo(try reader.takeDelimiter('\n'));
  whatever.bar = parseBar(try reader.takeDelimiter('\n'));
  return whatever;
}
1 Like
9

Spot the error :slight_smile:

stdin.err could be null, but you should still return an error in that case, this will panic in debug, undefined behavior in release

Anyway, yes, that’s possible, unless you have nested readers, as is common networking. It’s possible you have TCP stream → TLS stream → HTTP chunked format reader → application code. Where do you recover the error from? You try all of them? If so, in which order?

10

Absolutely no error! This is documented in std.Io.Reader.Error:

See the Reader implementation for detailed diagnostics.

No. It would be a bug on the standard library side to return error.ReadFailed without setting the err field in the implementation. They always make sure it is not null. Example.

std.http.Client.Connection.getReadError exists for some reason. I do not know if it is relevant. I have not dug into networking yet.

1 Like
11

Maybe we have different tolerance for crashes, but I wouldn’t trust the stdlib not having bugs. Here is one error.ReadFailed that does not set r.err.

https://codeberg.org/ziglang/zig/src/branch/master/lib/std/Io/File/Reader.zig#L255

You could claim that never happens in practice, and you would be probably right, but once you run a 24/7 server at scale, many unthinkable error conditions happen, so I don’t like depending on the chance.

2 Likes
12

I would expect a reader that supports nesting to be able to say “I failed because my child reader failed”. I.e., in its reader.err set it would have some error.ChildReaderFailed, which would have me check its reader.child.

So yes, I would want to check all of them, in the order from the outermost to the innermost, until I find an error that doesn’t tell me to check the next reader in line.

With that said, the status quo does seem a tad brittle. As demonstrated, bugs where error.ReadFailed is returned without setting reader.err do seem to happen, and what’s more, they apparently happen not only in user code, but also in the std. But I’m not sure what the right solution would be. I can’t think of a language feature that would prevent this.

I’m thinking maybe better conventions are what we need – a Reader implementor might want to establish that to return an error, they always have to use a function, e.g. have a method on std.Io.File.Reader:

// ALWAYS use this to return a read error!
fn fail(self: *std.Io.File.Reader, err: std.Io.File.Reader.Error) error{ReadFailed} {
    self.err = err;
    return error.ReadFailed;
}

// A very fictional example function for demonstration purposes
fn fallibleReadByte(self: *std.Io.File.Reader) error{ReadFailed}!u8 {
    if (!self.isByteAvailable())
        return self.fail(error.ByteUnavailable);
    return self.readByteInternal();
}

and consider directly returning error.ReadFailed a bug – have oneself ALWAYS use the function to return an error. That would then become easy to grep and fix in one go, I imagine.

As for swallowing error.Canceled, I honestly think once bugs in std are fixed and libraries (including std.http) move to using std.Io.Reader and std.Io.Writer as the actual interfaces, instead of taking pointers to their implementations, this will become not that much of an issue. I also don’t really agree with promoting error.Canceled to be a part of std.Io.Reader/Writer – I reckon there are many more possible implementations to which cancelation doesn’t directly apply than ones to which it does.

1 Like
13

Did you notice r.mode = .failure; line? You could only get there if you run into the SeekError earlier. You would return file_reader.err orelse file_reader.seek_err.? then. I did not do that because my example did not involve seeking.

I want my program to crash if I ever make incorrect assumptions. I like safety checks.

return stdin.err orelse error.Unexpected; or do not use any library that you do not trust.

1 Like
14

About this helper function:

One thing I am doing in my code at this point is having a helper function to retrieve the error from file readers (and something similar for file writers):

fn retrieveFileReaderError(reader: *const std.Io.File.Reader) (std.Io.File.Reader.Error || std.Io.File.Reader.SizeError || std.Io.File.Reader.SeekError) {
    if (reader.err) |err| return err;
    if (reader.size_err) |err| return err;
    if (reader.seek_err) |err| return err;
    std.debug.panic("{s} called but no error set", .{ @src().fn_name });
}

As a sidenote: Does somebody actually know when to use which error field or why there are multiple error fields in the first place? I could understand two fields (one for streaming and one for positional operation), but why 3? Streaming-only, positional-only and common?

15

Are you actually supposed to inspect size_err and seek_err? From a quick glance at the source code I get the impression that the two are just a sort of cache for errors to be returned by the getSize and seekTo functions, respectively, so that they keep returning the same error after they’ve failed at least once (or are not applicable to the reader’s mode).

Readers don’t provide any API for seeking or getting the size, so I don’t think anything other than err is meant to be relevant when you get an error.ReadFailed.

1 Like
16

I don’t know.

There’s no documentation about the fields or really how you are supposed to do error handling with with the File.{Reader,Writer} types.

So I just use this because at least it’s better than just letting error.ReadFailed/error.WriteFailed bubble up.

17

That’s the thing, though. You are supposed to let error.ReadFailed/error.WriteFailed bubble up. Use the std.Io.Reader and/or std.Io.Writer interfaces exclusively as much as possible, try all operations on them, then handle the implementations’ errors at the level that “owns” the implementations. Avoid handing implementations to downstream functions.

The idea is that you decouple the reading/writing logic from the particular implementations as much as possible, so that you can make your code more general and testable.

4 Likes
18

No, I mean bubble up as in letting error.{Read,Write}Failed up further up than the implementations.

EDIT: Here an example of what I mean with that:

const std = @import("std");

pub fn main(init: std.process.Init) !void {
    try sayHello(init.io);
}

fn sayHello(io: std.Io) !void {
    var buffer: [1024]u8 = undefined;
    var stdout: std.Io.File.Writer = .init(.stdout(), io, &buffer);
    try sayHelloInner(&stdout.interface);
}

fn sayHelloInterface(writer: *std.Io.Writer) !void {
    try writer.writeAll("Hello\n");
    try writer.flush();
}
1 Like
20

Found another hard case. Proper TLS client actually needs to do writes in the reader, because the TLS server can request key update or other bidirectional message, so if you try to abstract the TLS client over *std.Io.Reader/Writer as is common, and you do tls_reader.interface.readSliceShort() you actually might need to reach into tcp_writer.err to get the error.Canceled.