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

Enable thread_pool function to throw errors #20260

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

ippsav
Copy link
Contributor

@ippsav ippsav commented Jun 11, 2024

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 ippsav requested a review from kprotty as a code owner June 11, 2024 13:14
@ippsav ippsav force-pushed the fix_thread_pool_function_error branch 2 times, most recently from a551efb to 261c1ed Compare June 11, 2024 13:35
@alexrp
Copy link
Member

alexrp commented Oct 10, 2024

This needs a rebase to deal with the merge conflict.

@ippsav
Copy link
Contributor Author

ippsav commented Nov 10, 2024

@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 ippsav force-pushed the fix_thread_pool_function_error branch from 261c1ed to cd204ed Compare November 11, 2024 14:54
@kprotty kprotty merged commit d346d07 into ziglang:master Nov 11, 2024
10 checks passed
andrewrk added a commit that referenced this pull request Nov 11, 2024
This reverts commit d346d07.

I would like a chance to review this, please.
@andrewrk
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants