Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sync stdout run into unreachable code #16735

Closed
CGQAQ opened this issue Aug 8, 2023 · 7 comments
Closed

sync stdout run into unreachable code #16735

CGQAQ opened this issue Aug 8, 2023 · 7 comments
Labels
bug Observed behavior contradicts documented or intended behavior

Comments

@CGQAQ
Copy link

CGQAQ commented Aug 8, 2023

Zig Version

zig trunk

Steps to Reproduce and Observed Behavior

https://godbolt.org/z/7qEnf35fv

const std = @import("std");

pub fn main() !void {
    try std.io.getStdOut().sync();
}

image

Expected Behavior

At least should not reach unreachable code

@CGQAQ CGQAQ added the bug Observed behavior contradicts documented or intended behavior label Aug 8, 2023
@nektro
Copy link
Contributor

nektro commented Aug 8, 2023

per https://stackoverflow.com/questions/26257171/flush-kernel-buffers-for-stdout stdout does not support fsync(2) which is why it returns EINVAL and zig uses unreachable to signify programmer error

@CGQAQ
Copy link
Author

CGQAQ commented Aug 8, 2023

zig uses unreachable to signify programmer error

why we already have the error union, but we instead use unreachable to signify programmer error, then there's no chance people can catch the error

@rohlem
Copy link
Contributor

rohlem commented Aug 8, 2023

then there's no chance people can catch the error

@CGQAQ unreachable is used in two scenarios:

  • Errors which cannot be reported reliably. Declaring that X is an error, but only triggering in 50% of the cases X happens, can often lead to a 50% buggy program.
    At least crashing in these instances signals that something is severely wrong, and the code needs to be fixed (to check for X in some 100% reliable way).
  • (This situation:) Errors which are expected to never occur when the program is written correctly.
    Here unreachable is also meant to make you look up "why did this happen, what do I need to do to fix my code?".

If we added the error to the error set, that means that all correct programs (which don't sync these files / under these conditions) pay a performance cost, or have to explicitly declare "Yes, I guarantee I'm not fsync-ing stdout" at every call site / in a wrapper with catch |err| switch(err) { else => |e| e, BADF => unreachable }.
(I personally like to use inline comments to explain the reasons why a place in code is marked unreachable, which can help point out what to do differently / why something doesn't work. In this case we could add a comment saying which types of files can't be fsync-ed.)

@CGQAQ
Copy link
Author

CGQAQ commented Aug 8, 2023

Got it, thanks

@CGQAQ CGQAQ closed this as completed Aug 8, 2023
@neurocyte
Copy link

  • (This situation:) Errors which are expected to never occur when the program is written correctly.
    Here unreachable is also meant to make you look up "why did this happen, what do I need to do to fix my code?".

I really don't agree with this. The assertion that INVAL from fsync implies that your program is somehow not "correct" is not valid. How should a program know what capabilities the standard file descriptors have? They may well be pointing pretty much anywhere. What if it's not one of the standard FDs but some other FD duped to stdout? Calling something like fsync may make sense, and if the FD does not support it, it may also make sense to ignore the error.

I don't see how the standard library can (or should) determine that this implies that the program is incorrect. Zig is a systems programming language and handling unexpected errors from the OS is something that should be promoted, not something that should be made difficult.

I would go as far as to say BADF is not even something that should be unreachable. It may well be that the FD was opened by some other process outside the programmers control and not in a well known state. I would much rather be able to handle this and give some constructive feedback to my users than give them a panic.

@rohlem
Copy link
Contributor

rohlem commented Aug 8, 2023

How should a program know what capabilities the standard file descriptors have?

@neurocyte I imagine it's possible to query those exact capabilities using additional syscalls. I personally don't have a lot of experience with POSIX handles though.
See also previous (still-open) discussion in #6389 .

@neurocyte
Copy link

I'm pretty sure that calling fsync and checking for INVAL is the only standard way to check for fsync support. This is a pretty common pattern for posix APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

No branches or pull requests

4 participants