Fix eof() and read*() behaviour for ::File and ::LibuvStream

[samoconnor]

See new test at end of test/file.jl

See new test/read.jl

[samoconnor]

It looks like this new test is failing on Win32

    From worker 2:    Expression: readall(io()) == text
    From worker 2:     Evaluated: "C1,C2 1,2 a,b" == "C1,C2\n1,2\na,b\n"

Maybe a DOS line-ending issue?

Could someone who has windows please look at this?

edit: I think this is resolved now, the last appveyor build ran fine except for a crash deleting a tmp directory due to open files, running again now with a fix for that...

[tkelman]

Please separate the introduction of new methods from bug fixes into separate PRs.

[samoconnor]

Appveyor now says rmdir: Directory not empty after a mktempdir() do dir ... end block ??

[samoconnor]

separate the introduction of new methods from bug fixes

Adding the missing methods is the bug fix.

Handling uv_fs_read() EOF requires eof(::File) because calling eof(::File) is a precondition of calling read(::File, ::UInt8). That in-turn requires skip(::File), and you can't really have skip(::File) without having seek(::File) and seekend(::File) as well.

[tkelman]

Adding the missing methods is the bug fix.

Then what was the purpose of opening the PR with a single commit, then adding additional content to it? The new test in the first commit didn't seem to need these new methods to pass, so why are these in the same PR?

[samoconnor]

@vtjnash, does this fix look right to you?

[vtjnash]

just merged this

[samoconnor]

Nice one. I've just rebased.

[samoconnor]

Thanks to @mmaxs for #14630 which found a LibuvStream gap in the coverage of test/read.jl. This PR (#14699) now incorporates a revised version of @mmaxs fix from #14630.

[samoconnor]

Windows now passing CI.
Linux i686 is passing CI.
Linux x86_64 is failing early in the build with make[1]: *** [julia-deps] Error 2

[tkelman]

If this can't run on travis, chances are it can't run on distro buildbots either. Not a good idea to special-case for CI like this.

[samoconnor]

Most definitely agree!
Just attempting to discover which part of the code is causing deadlock...

[tkelman]

ahk. did you track it down? I'm finding TCPSockets kind of challenging to add tests for myself. This branch helps get MbedTLS working again, though I'm not sure what its impacts are on the optimizations Jameson had been working towards.

[samoconnor]

I hope that this yield() will fix the deadlock. Waiting for Travis to start up to find out...

[tkelman]

the commit samoconnor@29e5455 2161af4 that has nothing to do with File would be better suited for its own separate pr.

[tkelman]

remaining untested methods from #14608:

• readlines of a filename
• readuntil of a filename
• eachline of a filename
• write(::IO, ::IO) for IO types that are not subtypes of AbstractIOBuffer

not-directly-tested newly introduced methods:

• readavailable

[tkelman]

the strange spacing is really distracting here

[samoconnor]

I've just taken out most of the temporary cleanup() calls.
That was an attempt to figure out why CI was hanging (i.e. maybe because of too many tasks/file handles/sockets ...).

[samoconnor]

@tkelman: untested newly introduced methods: readavailable

You're talking about these right?

readavailable(io::File) = read(io)
readavailable(io::AbstractIOBuffer) = read(io)
readavailable(s::IOStream) = read!(s, Vector{UInt8}(nb_available(s)))

... called from here:

function write(to::IO, from::IO)
    while !eof(from)
        write(to, readavailable(from))
    end
end

...which is tested with 15 combinations of to and from stream types here.

What, in your opinion, would be an adequate test???

[tkelman]

That's fine. An indirect test is better than no test, but that's literally the only call site of readavailable other than one line in examples/clustermanager/0mq/ZMQCM.jl.

[samoconnor]

All the CI is passing now. (with #14747 test disabled).

Passing tests cover something like:
{filename, File, IOStream, IOBuffer, TCPSocket, PipeEndpoint}
X {read(s, T), read!(s) read!(s, a), eof(s), readstring(s), read(s), read bytes!(s, a), readuntil(s,c), readline(s), readlines(s), eachline(s), countlines(s), readcsv(s), position(s), seek(s,n), skip(s), seekend(s)}
X {short CSV string, + 4 x longer test strings approx SZ_UNBUFFERED_IO in length.}

[samoconnor]

... that's literally the only call site of readavailable other than ...

I think I understand your concern. I think that readavailable should eventually be deprecated (or not exported).
See #14546 (comment) and #7478.

I also think that this generic implementation of write(to::IO, from::IO) should almost never be used once appropriate stream-type specialised methods are added. e.g. specialised methods should use sendfile in some cases or maybe mmap in others.

[tkelman]

It is pretty widely used in packages according to github code search, which is most of my concern about it not being directly unit-tested here. So un-exporting it or deprecating it would break code / introduce warnings outside of base but not the tests within base (still probably worth the cleanup to do at some point though). "untested" was imprecise and unfair wording, sorry.

[samoconnor]

ready to merge?

[vtjnash]

It is pretty widely used in packages according to github code search...

that's annoying. i just read through the 12 pages of search results and found the following use cases for readavailable:

1. a few were correct reimplementations of sendfile (aka write(to::IO, from::IO)), half of the time to is another real IO object, the other half of the time, it's a pseudo-IO / display object (like a WebSocket or GUI console)
2. the rest were examples of broken code that might hang or print incomplete fragments if the output was non-trivial

[coveralls]

Changes Unknown when pulling 98744de on samoconnor:uv_fs_read_eof into ** on JuliaLang:master**.