Skip to content

std.process.cleanExit so we can not clean up our cake and eat it too #6395

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

Closed
andrewrk opened this issue Sep 22, 2020 · 8 comments
Closed

std.process.cleanExit so we can not clean up our cake and eat it too #6395

andrewrk opened this issue Sep 22, 2020 · 8 comments
Assignees
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@andrewrk
Copy link
Member

/// Indicate that we are now terminating with a successful exit code.
/// In debug builds, this is a no-op, so that the calling code's
/// cleanup mechanisms are tested and so that external tools that
/// check for resource leaks can be accurate. In release builds, this
/// calls exit(0), and does not return.
pub fn cleanExit() void {
    if (std.builtin.build_mode == .Debug) {
        return;
    } else {
        std.process.exit(0);
    }
}

Calling code would look something like this:

var foo = allocateResource();
defer cleanupResource(foo);

if (condition) {
    return std.process.cleanExit();
}
@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Sep 22, 2020
@andrewrk andrewrk added this to the 0.8.0 milestone Sep 22, 2020
@andrewrk andrewrk added the accepted This proposal is planned. label Sep 23, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.7.0 Sep 23, 2020
@andrewrk andrewrk self-assigned this Sep 23, 2020
andrewrk added a commit that referenced this issue Sep 23, 2020
As part of this:

 * add std.process.cleanExit. closes #6395
   - use it in several places
 * adjust the alignment of text in `zig build --help` menu
 * Cache: support the concept of "unhit" so that we properly keep track
   of the cache when we find out using the secondary hash that the cache
   "hit" was actually a miss. Use this to fix false negatives of caching
   of stage1 build artifacts.
 * fix not deleting the symlink hash for stage1 build artifacts causing
   false positives.
 * implement support for Package arguments in stage1 build artifacts
 * update and add missing usage text
 * add --override-lib-dir and --enable-cache CLI options
   - `--enable-cache` takes the place of `--cache on`
 * CLI supports -femit-bin=foo combined with --enable-cache to do an
   "update file" operation. --enable-cache without that argument
   will build the output into a cache directory and then print the path
   to stdout (matching master branch behavior).
 * errors surfacing from main() now print "error: Foo" instead of
   "error: error.Foo".
@joachimschmidt557
Copy link
Contributor

I think this has a potential to be a footgun if this function is not called from main():

const std = @import("std");

fn foo() void {
    return std.process.cleanExit();
}

pub fn main() !void {
    foo();
    std.debug.print("helo\n", .{});
}

On debug builds, this prints helo and on release builds, it prints nothing. This difference in behaviour may be very risky.

@ikskuh
Copy link
Contributor

ikskuh commented Sep 23, 2020

I don't think this is a good idea. Afaik if you don't clean up your sockets nicely, it will have effects when restarting the same program (afaik: if your process crashes, you cannot rebind the same socket for some minutes unless you use SO_REUSEADDR and then it's only possible with the same program). I don't think this is desirable as a default behaviour for release builds.

Also cleaning up resources with defer might do some more complex logic like notifying a server of a clean shutdown

@BarabasGitHub
Copy link
Contributor

Also cleaning up resources with defer might do some more complex logic like notifying a server of a clean shutdown

I was also thinking about this, but should that really be in a defer? There's no way to handle errors in defer statements, so it might already never send the shutdown message.

I do agree that it looks fishy. I'd rather call it uncleanExit or hardExit or something which indicates that it's a hard shutdown without any cleanup. And indeed I'm not sure cleaning up in debug is a good idea.

@andrewrk andrewrk removed the accepted This proposal is planned. label Sep 23, 2020
@joachimschmidt557
Copy link
Contributor

Alternative solutions to std.process.cleanExit:

  1. Change the definition and implementation of a defer statement to run code right before control flow leaves a scope.
  2. Discourage the use of std.process.exit and instead use return values of main

For 1, the new definition would mean that calling a noreturn function such as std.process.exit would qualify as leaving the scope, whereupon the defer statements would be run.

In 2, this can be done:

pub fn main() !u8 {
    // instead of: std.process.exit(1);
    return 1;
}

@Rocknest
Copy link
Contributor

@joachimschmidt557 see #2198 (comment) for case (1).

@thejoshwolfe
Copy link
Contributor

i ran into a use case for this here: https://github.com/andrewrk/groovebasin/blob/b27b4e02baa13e88d601e8008fd9d59b55d7ff22/tools/paste-js.zig#L59-L60

It does me no good to meticulously deallocate all my memory when the process is going to exit anyway. Is there a reason to close my open file descriptors before my process exits? Pretty sure the OS will do that for me. (I don't have any unflushed output buffers to worry about.)

I'm not an expert on SO_REUSEADDR, but I don't see any evidence in the documentation that it's affected by the application closing the socket before exiting. Someone please correct me if I'm wrong.

@thejoshwolfe
Copy link
Contributor

The name "cleanExit" is one problem with this proposal. At first glance, that sounds like it's going to run a bunch of atexit-like deinitialization code. If i wanted to shutdown my process quickly and abruptly, I would not reach for something named "cleanExit".

Another problem is the intended way to use this function is unintuitive. You're supposed to return it from main, but the function signature doesn't indicate that. In order to make this function signature not a footgun, we could change the way main() declarations work to have a special return type fn main() !std.process.ExitStats { which is returned by fn cleanExit() ExitStatus {. That gives the programmer the intuition to connect the dots. However, I'm not sure we want to disrupt main() like that just for this feature.

@andrewrk
Copy link
Member Author

I think the intended usage pattern of this is too subtle for it to go into std. It's 4 lines, no problem for application developers to provide. Thanks all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

6 participants