stream: fix reading LibuvStream into array

[vtjnash]

Adds a new abstraction take!(dst::Array{T,N}, src::Array{T,N}) for doing an efficient copyto! equivalent. Previously it was assumed that compact did this automatically, which wasn't a great assumption.

Fixes #56078

[nhz2]

This is still broken.

readbytes! is documented to never decrease the size of the input vector, but in this example, it gets changed from 1024 to 10.

function mwe(filepath, nb, max_nb)
    write(filepath, ones(UInt8, nb))
    p = open(`cat $filepath`; read=true)
    b = Vector{UInt8}(undef, 1024)
    nr = readbytes!(p, b, max_nb)
    return nr, b
end
nr, b = mwe(tempname(), 10, 10000)

(10, UInt8[0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01])

[ararslan]

Adds a new abstraction take!(dst::Array{T,N}, src::Array{T,N}) for doing an efficient copyto! equivalent.

I don't understand how this is intended to differ meaningfully from copyto!. Also, isn't this adding a new feature (in the form of a new method for an exported function) that would need to be backported to a patch release in order to fix the regression in 1.11?

[ararslan]

I'm confused, the "needs tests" label was removed but tests weren't added. It would be good to include a test to ensure #56078 is indeed fixed and to prevent it from regressing in the future.

Since this is adding methods to an exported function, it would be good to document the methods. If they aren't intended to be user-facing, perhaps make it a separate, internal-only function?

[vtjnash]

I wasn't sure about what it needed for a new test here, and didn't seem worth spending time on it, while the followup issues was already caught by the existing read tests. You are right that these new functions would be internal-only, by policy, for v1.11 so they should get documented separately on master. But assuming these are the names we like, then they can already be added internally like this.

[KristofferC]

Why can't these just be internal functions over extending take!? Keep the bugfix separate from any new API.

[ararslan]

I wasn't sure about what it needed for a new test here

Would it work to use the test case from #56078? Just guessing but this might do it:

function issue56078(filepath, nb, max_nb)
    write(filepath, ones(UInt8, nb))
    @static if Sys.iswindows()
        cmd = `cmd.exe /C type $filepath`
    else
        cmd = `cat $filepath`
    end
    p = open(cmd; read=true)
    b = Vector{UInt8}(undef, 1024)
    nr = readbytes!(p, b, max_nb)
    return nr, b
end
mktemp() do file, io
    close(io)
    nr, v = issue56078(file, 53209, typemax(Int))
    @test nr == 53209
    @test length(v) > 1024
end

Why can't these just be internal functions over extending take!?

We could add methods to take! later that would simply call the internals added in service of this issue.

[vtjnash]

I couldn't think of a better name, since they do what take! does, unless take!! is somehow better since we mutate both arguments?

[KristofferC]

Call it _take! then. Whatever to decouple the bugfix from the new feature.