zig icon indicating copy to clipboard operation
zig copied to clipboard

Enable thread_pool function to throw errors

Open ippsav opened this issue 1 year ago • 1 comments

Function passed to thread_pool causes build failure when the return type is !void

Fixed with the help of @Cloudef

code:

const std = @import("std");

const ThreadPool = std.Thread.Pool;

pub fn main() !void {
    var pool: ThreadPool = undefined;

    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();

    defer {
        _ = gpa.deinit();
    }

    const values: [10]i32 = .{ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

    try pool.init(.{ .allocator = allocator, .n_jobs = 4 });
    defer pool.deinit();

    for (values) |v| {
        try pool.spawn(addOneAndPrint, .{v});
    }
}

fn addOneAndPrint(i: i32) !void {
    std.debug.print("{d}\n", .{i + 1});
}

Default behavior in 0.12/0.13:

run
└─ run t_pool
   └─ zig build-exe t_pool Debug native 1 errors
/Users/ippsav/zig/lib/std/Thread/Pool.zig:93:13: error: error is ignored
            @call(.auto, func, closure.arguments);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ippsav/zig/lib/std/Thread/Pool.zig:93:13: note: consider using 'try', 'catch', or 'if'
referenced by:
    spawn__anon_2804.Closure: /Users/ippsav/zig/lib/std/Thread/Pool.zig:88:58
    runFn: /Users/ippsav/zig/lib/std/Thread/Pool.zig:92:29
    remaining reference traces hidden; use '-freference-trace' to see all reference traces
error: the following command failed with 1 compilation errors:
/Users/ippsav/zig/zig build-exe -ODebug -Mroot=/Users/ippsav/Development/zig/t_pool/src/main.zig --cache-dir /Users/ippsav/Development/zig/t_pool/zig-cache --global-cache-dir /Users/ippsav/.cache/zig --name t_pool --listen=-
Build Summary: 2/7 steps succeeded; 1 failed (disable with --summary none)
run transitive failure
└─ run t_pool transitive failure
   ├─ zig build-exe t_pool Debug native 1 errors
   └─ install transitive failure
      └─ install t_pool transitive failure
         └─ zig build-exe t_pool Debug native (reused)
error: the following build command failed with exit code 1:
/Users/ippsav/Development/zig/t_pool/zig-cache/o/199ec97c8a7f00b3119749417699bd69/build /Users/ippsav/zig/zig /Users/ippsav/Development/zig/t_pool /Users/ippsav/Development/zig/t_pool/zig-cache /Users/ippsav/.cache/zig --seed 0xe9c27c51 -Zab13babf81632322 run

With the changes:

❯ rzig build run
3
5
6
4
2
7
8
11
10
9

In the case of an error thrown:

❯ rzig build run
11
7
6
error: RandomError
8
10
9
4
3
2
/Users/ippsav/Development/zig/t_pool/src/main.zig:27:9: 0x1003611b7 in addOneAndPrint (t_pool)
        return error.RandomError;
        ^
        ```

ippsav avatar Jun 11 '24 13:06 ippsav

This needs a rebase to deal with the merge conflict.

alexrp avatar Oct 10 '24 13:10 alexrp

@alexrp Not sure if this is the right way to approach this issue from what i've understood while having a discussion with someone about this, what do you think of the changes ?

ippsav avatar Nov 10 '24 15:11 ippsav

I recognize that this matches thread spawning behavior as well as the main thread start logic, however, I think it is going to hide bugs 99% of the time and it is generally better to use void for the return type of worker functions used with thread pool.

Your addOneAndPrint example above is not a real world use case. Do you have one? Personally, all of my real applications of ThreadPool benefits from enforcing void return type.

andrewrk avatar Nov 11 '24 22:11 andrewrk