`@sync (@spawn @spawn f(); @spawn @spawn g())` is not thread-safe?

[tkf]

@sync @spawn @spawn f() expands roughly to

let sync = Any[]
    task1 = Task() do
        task2 = Task() do
            f()
        end
        task2.sticky = false
        push!(sync, task2)
        schedule(task2)
        task2
    end
    task1.sticky = false
    push!(sync, task1)
    schedule(task1)
    task1
end

This code itself is OK. However, I don't think the following code is thread-safe:

@sync begin
    @spawn @spawn f()
    @spawn @spawn g()
end

This is because push!(sync, task2) for @spawn f() and @spawn g() can be executed in parallel. This came up in #34543 (comment)

How should it be fixed? Maybe expand @spawn ... to something like below?

if $(Expr(:islocal, :sync))
    local sync_sub = Any[]
    task = Task() do
        local sync = sync_sub
        ...
    end
else
    task = Task() do
        ...
    end
end
task.sticky = false

if $(Expr(:islocal, :sync))
    push!(sync, task)
    @async begin
        wait(task)
        append!(sync, sync_sub)  # merge sub-tasks
    end
end

schedule(task)

[JeffBezanson]

A simpler approach would be to lock it, and repeatedly pop a task from the vector and wait for it until the vector is empty.

I assume that sync should wait for every spawn inside it, but maybe a case can be made that it should only wait for the outermost level. The reason would be that then you know the full set of tasks to wait on by the time you reach the end of the sync block. Is that property useful at all?

[tkf]

Actually this (rather contrived) example shows that the approach I posted above is not a correct fix:

fib(x) =
    @sync begin
        function f(x)
            x < 0 && error("Please give me an integer.")
            x in (0, 1) && return x
            a = @spawn f(x - 2)
            b = @spawn f(x - 1)
            return fetch(a) + fetch(b)
        end
        f(x)
    end

But I think it's conceivable to use a recursive closure with @spawn wrapped with @sync like this so that you get a good error report. In such case, I think it makes sense to track all the nested tasks.

I assume that sync should wait for every spawn inside it, but maybe a case can be made that it should only wait for the outermost level.

I'm kind of hoping that Julia gets structured concurrency #33248 some day :) So, it would be nice to avoid "leaking" tasks at least when using explicit structured concurrency pattern like @sync.

(cc @c42f)

[tkf]

Is locked Vector better than Channel(Inf) in terms of performance?