Assert triggered in std.Io.Task.start

Help
,
1

I’ve been consistently running into an assert in the standard library. I’ve been trying to figure out what is causing it, and I have been unable to reduce it to a small reproduction so I can write up a bug report on the Ziglang repository.

Stack trace:

thread 588214 panic: reached unreachable code
/home/southporter/.local/share/mise/installs/zig/0.16.0/lib/std/debug.zig
:420:14: 0x1399fe9 in assert (std.zig)
    if (!ok) unreachable; // assertion failure
             ^
/home/southporter/.local/share/mise/installs/zig/0.16.0/lib/std/Io/Threaded.zig:560:19: 0x150d23c in start (std.zig)
            assert(old_status.num_running > 0);
                  ^
/home/southporter/.local/share/mise/installs/zig/0.16.0/lib/std/Io/Threaded.zig:1797:29: 0x150b663 in worker (std.zig)
            runnable.startFn(runnable, &thread, t);
                            ^
/home/southporter/.local/share/mise/installs/zig/0.16.0/lib/std/Thread.zig:422:13: 0x150b215 in callFn__anon_27338 (std.zig)
            @call(.auto, f, args);
            ^
/home/southporter/.local/share/mise/installs/zig/0.16.0/lib/std/Thread.zig:752:30: 0x150b009 in entryFn (std.zig)
                return callFn(f, args_ptr.*);
                             ^
???:?:?: 0x7f455d46bc18 in start_thread (/lib64/libc.so.6)
???:?:?: 0x7f455d4ef5cb in __clone3 (/lib64/libc.so.6)

It happens fairly consistently in the following code:

pub fn load(image: *Image, group: *std.Io.Group, client: *std.http.Client) void {
    switch (image.state) {
        .unloaded => |kind| {
            if (kind != .init) return;
            image.state.unloaded = .in_flight;
            // This line _seems_ to be the culprit
            group.concurrent(client.io, fetch, .{ image, client }) catch |err| {
                log.warn("Image loading failed: {t}", .{err});
                image.state.unloaded = .err;
            };
        },
        .downloaded => {},
    }
}

Full codebase file: https://codeberg.org/Southporter/hush/src/commit/931dba033b5285f2fb4421dfc9cbe5a1f2585bb1/src/Image.zig#L93

I looked into the std.Io.Task code for the start, and it seems like this is comming back with a old_status.num_running == 0:

            const old_status = group.status().fetchSub(.{
                .num_running = 1,
                .have_awaiter = false,
                .canceled = false,
            }, .acq_rel); // acquire `group.awaiter()`, release task results
            assert(old_status.num_running > 0);

It seems like I am somehow getting into a state where there are 0 tasks running and this is somehow unexpected.

It’s also possible I’m using it wrong.

Any insights into how to reduce to a small reproduction this would be helpful.

2

Sorry for not just following the link, but can you talk a little about what this group represents in your code? I ask because the design is kind of confusing: from the context you gave, nobody appears to be waiting for fetch to finish, although arguably that should be the job of a function named load.

3

The design is to spawn the fetch to an Io.Group and let it complete on it’s own. Nothing waits for it, it updates the state of the Image and then the UI reads that updated state when it happens.
Basic flow is:

std.http.Client.fetch the full html page. This is a sync call from the main thread.
Parse out the pieces including Images.
When an image placeholder is clicked, spawn a concurrent task that does the image loading.

It’s more of a fire-and-forget model. From the std.Io.Group docs:

The resources associated with each task are guaranteed to be released when the individual task returns, as opposed to when the whole group completes or is awaited. For this reason, it is not a resource leak to have a long-lived group which concurrent tasks are repeatedly added to.

So it seems like I can add them to the group, let them run to completion, and I don’t have to await() them individually, but I can cancel the group to terminate downloads when a page change happens or something like that.

4

It’s hard to tell from the code linked exactly, but I’d advise trying to catch the callsite, or minify the repro.

The assert is checking if num_running > 0 which seems to not sanely happen in the group’s normal code paths, so indicates potential re-use, incorrect copy, use after free, or something similar. Check whether your group or Page aren’t being destroyed, moved, or accessed from multiple threads.

5

I’m trying to minify the repro, but haven’t been able to get it to trigger in any predictable way other than running my full application.

The std.Io.Group is stable. I make sure to cancel the group whenever I tear down a page.

Can you ellaborate? I’m not sure what you mean here?

6

Ah, apologies! Should have been more specific. Catching the problematic concurrent call in a debugger is what I was suggesting. At that point you could check whether the group is truly still valid or in some weird state.

Catching the assert too could help reveal what’s happened in your other code in the mean time that might have invalidate the group state.

I’m finding it a little hard to follow the code paths myself just by reading, would have loved to have been able to mentally map out your Group usage and give a solid answer right away!

I guess another thing - is the assert the only error? Is it possible something else is in the process of error handling or cleanup while this thread is being spawned, causing the Group to get tangled while the true issue has already occured.

7

Thanks for this lead. I think I found the problematic code:

fn viewFrame(hush: *Hush, max_width: i32) void {
    std.debug.assert(hush.page != null);
    var p = hush.page orelse unreachable; // This line is the issue
    // Should be: const p = &(hush.page orelse unreachable);

    {
        const scroll = dvui.scrollArea(@src(), .{}, .{ .expand = .horizontal, .background = true, .max_size_content = .width(@floatFromInt(max_width)) });
        defer scroll.deinit();

        log.info("====== STARTING HTML WALK =======", .{});
        hush.renderPage(&p);
        log.info("====== END HTML WALK ============\n", .{});
        hush.renderLinks(&p);
    }
}

I unintentionall moved the Group here. The page is copied and then I take the address of the copy instead of getting the address of the stable page.
Now that I found it, it seems like a dumb mistake. The assert is not the problem, it’s my code.

Thanks @pasta and @alanza for your help!

4 Likes
8

I think I would just write this instead:

std.debug.assert(hush.page != null);
if(hush.page) |*p| {
    const scroll = ...
    ...
}

You have the curlies and indentation there already anyway, why not make it into a full if?
With the assert the compiler should have enough information to optimize it.

2 Likes
9

I agree, I definitely would prefer .? instead of orelse unreachable since they should be equivalent here (if not everywhere?).

could also do if (hush.page) |*p| { ... } else unreachable

1 Like
10

Yes, this is a better/cleaner way. This probably got caught up in a refactor and morphed from there. It is currently messy.

1 Like