while(true) getting removed when building for release mode (?)

[AlphaTechnolog]

Zig Version

0.13.0

Steps to Reproduce and Observed Behavior

Hi! I observed this issue while creating a program which made use of two threads that consume a queue ds, so the first thread will add allocated data to the queue while the second thread will consume it. First of all am sorry for the long repro program, this was the best i could do to try to reproduce it since others simplified attempts would just fail to reproduce the issue. Maybe experts can help me here understand a little bit why this is behaving the way it's behaving?

This is the example program i ended writing for repro purposes

const std = @import("std");

const heap = std.heap;
const mem = std.mem;
const io = std.io;
const Thread = std.Thread;

pub fn Queue(comptime Child: type) type {
    return struct {
        const This = @This();

        const Node = struct {
            data: Child,
            next: ?*Node,
        };

        gpa: std.mem.Allocator,
        start: ?*Node,
        end: ?*Node,

        pub fn init(gpa: std.mem.Allocator) This {
            return This{
                .gpa = gpa,
                .start = null,
                .end = null,
            };
        }

        pub fn enqueue(this: *This, value: Child) !void {
            const node = try this.gpa.create(Node);
            node.* = .{ .data = value, .next = null };
            if (this.end) |end| end.next = node //
            else this.start = node;
            this.end = node;
        }

        pub fn dequeue(this: *This) ?Child {
            const start = this.start orelse return null;
            defer this.gpa.destroy(start);
            if (start.next) |next|
                this.start = next
            else {
                this.start = null;
                this.end = null;
            }
            return start.data;
        }
    };
}

const Person = struct {
    name: []const u8,
    lastname: []const u8,
    allocator: mem.Allocator,

    pub fn init(alloc: mem.Allocator, name: []const u8, lastname: []const u8) !*Person {
        const instance = try alloc.create(Person);
        instance.* = Person{ .name = name, .lastname = lastname, .allocator = alloc };
        return instance;
    }

    pub fn greet(self: Person) !void {
        const stdout = io.getStdOut().writer();
        try stdout.print("hello {s} {s}\n", .{ self.name, self.lastname });
    }

    pub fn deinit(self: *Person) void {
        self.allocator.destroy(self);
    }
};

pub fn main() !void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    defer if (gpa.deinit() == .leak) @panic("memleak!\n");

    const allocator = gpa.allocator();

    var queue = Queue(*Person).init(allocator);

    const Enqueuer = struct {
        const Args = struct {
            alloc: mem.Allocator,
            pqueue: *Queue(*Person),
        };

        fn handleFnImpl(args: Args) !void {
            while (true) {
                try args.pqueue.enqueue(try Person.init(args.alloc, "One", "More"));
                std.debug.print("[1] waiting {d}\n", .{2e9});
                std.time.sleep(2e9);
            }
        }

        fn handleFn(args: Args) void {
            handleFnImpl(args) catch |err| @panic(@errorName(err));
        }
    };

    const Handlers = struct {
        const Args = struct {
            pqueue: *Queue(*Person),
        };

        fn handleFnImpl(pqueue: *Queue(*Person)) !void {
            while (true) {
                if (pqueue.dequeue()) |person| {
                    std.debug.print("[2] greeting ", .{});
                    try person.greet();
                    person.deinit();
                }
            }
        }

        fn handleFn(args: Args) void {
            handleFnImpl(args.pqueue) catch |err| @panic(@errorName(err));
        }
    };

    const enqueuer = try Thread.spawn(.{}, Enqueuer.handleFn, .{Enqueuer.Args{
        .pqueue = &queue,
        .alloc = allocator,
    }});

    defer enqueuer.join();

    _ = try Thread.spawn(.{}, Handlers.handleFn, .{
        Handlers.Args{ .pqueue = &queue },
    });
}

This program works fine if one runs it by using zig build run in an empty initialised project, and produces the next output:

[1] waiting 2000000000
[2] greeting hello One More
[1] waiting 2000000000
[2] greeting hello One More
...

The problem happens when building with -Doptimize=Release{Safe,Fast,Small} flags, if one does something like this

zig build -Doptimize=ReleaseSafe
./zig-out/bin/example-app

It will just omit the instructions to .dequeue() in the second thread, and produce the next:

[1] waiting 2000000000
[1] waiting 2000000000
[1] waiting 2000000000

So messing a little bit i observed that updating the second thread code to something like this would make it work in release mode aswell

        fn handleFnImpl(pqueue: *Queue(*Person)) !void {
            while (true) {
                std.debug.print("??\n", .{});
                if (pqueue.dequeue()) |person| {
                    std.debug.print("[2] greeting ", .{});
                    try person.greet();
                    person.deinit();
                }
            }
        }

And that will start printing both ?? and the hello {s} {s}, and also call deinit on *Person

so investigating i came up with this issue that was kinda similar but they suggested in some point doing

asm volatile ("nop")

And if i replace that std.debug.print("??") with asm volatile ("nop") it starts to work aswell just fine in release mode.

Expected Behavior

Tbh im not sure wether the optimization issue is because of the anonymous structs usage, or because of the structs as arguments, but anyways expected behavior i think would be that the while (true) doesn't gets removed because if i need to be dequeuing every time i need to do a while true prior the .dequeue, ik a while (queue.dequeue()) could do but that will end the execution when no more elements are found which is not my usecase.

[rohlem]

Without the use of any synchronization primitives (fences, atomics) the compiler currently doesn't consider the memory effects of other threads,
so optimizations are allowed to interfere with inter-thread communication.

As for the workarounds you found, std.debug.print internally locks a mutex, which does provides some synchronization.
asm volatile ("nop") probably ultimately hinders local compiler optimizations, making the resulting program re-read memory.

Zig's memory model currently mostly follows what LLVM provides to support the newest C and C++ standards (see also #6396),
so I would suggest you to familiarize yourself with the memory models of C and C++ and relevant example code on concurrency to understand how to equivalently employ those primitives in Zig.

EDIT: Also feel free to ask for advice in one of the community spaces, which can be more friendly to newcomers and can help differentiate what is or isn't a Zig bug.
Generally this issue tracker is focused on technical discussion of Zig toolchain development.

[AlphaTechnolog]

So this probably isn't a bug in the compiler but rather a lack of synchronization method when dealing with threads, do you think this should in that case be closed @rohlem? Also thanks for the resources provided, i've been investigating a little bit more about fences and atomics and atomics seems to be something i could try studying further for my usecase which is write/reading at the same time

[rohlem]

Discussion what the code is meant to do probably fits best into the above-linked memory model issue #6396 .
For discussion on runtime safety, i.e. adding safety checks that could detect the race condition and produce a runtime panic, there exists issue #1199 .
I currently don't see any alternative perspective or discussion points this issue provides. If you agree with that, I think it's safe to close it.

[AlphaTechnolog]

Discussion what the code is meant to do probably fits best into the above-linked memory model issue #6396 . For discussion on runtime safety, i.e. adding safety checks that could detect the race condition and produce a runtime panic, there exists issue #1199 . I currently don't see any alternative perspective or discussion points this issue provides. If you agree with that, I think it's safe to close it.

Yeah they look much more appropiate than this one, so, sure. Thank you!