FIFOBuffer eof() true when close() not called

[samoconnor]

I create a fifo buffer and start an async task to write to it every 1 second forever.
Then i try to read while !eof(), but eof() returns true after the first read (and i have not called close()).

julia> using HTTP

julia> io = HTTP.FIFOBuffers.FIFOBuffer()
HTTP.FIFOBuffers.FIFOBuffer(0, 4611686014132420609, 0, 1, 1, -1, 0, UInt8[], Condition(Any[]), Task (runnable) @0x00000001178c1450, false)

julia> @async while true
           println(io, now())
           sleep(1)
       end
Task (runnable) @0x00000001178c3850

julia> while !eof(io)
           println(String(readavailable(io)))
           @show eof(io)
       end
2017-08-22T21:53:39.083
2017-08-22T21:53:40.249
2017-08-22T21:53:41.256
2017-08-22T21:53:42.262

eof(io) = true

[quinnj]

Currently, this is by design. eof generally means "there are no more bytes to read from this stream", which is reflected in HTTP.FIFOBuffer' current behavior. There is new support for setting an "expected length", that will be reflected during FIFOBuffer processing (i.e. eof won't return true until X number of bytes have been processed). So you could do

io = HTTP.FIFOBuffers.FIFOBuffer()
io.expectedlength = typemax(Int64)
@async while true
    println(io, now())
    sleep(1)
end

while !eof(io)
    println(String(readavailable(io)))
    @show eof(io)
end

[samoconnor]

The documentation says: "If the stream is not yet exhausted, this function will block to wait for more data if necessary, and then return false.".

"stream exhausted" is different to "buffer temporarily empty".
For a FIFO, "stream exhausted" === "buffer permanently empty" === (("close called" or "unrecoverable error") and "buffer empty").

FIFOBuffer seems like the wrong place to know "Content-Length". I think it might be better to have knowledge of the "expected length" in the code that reads from the TCP socket. It should call write() on the FIFO when data arrives, then call close() on the FIFO when expected length is reached.

[samoconnor]

See BufferStream example

function eof(s::BufferStream)
    wait_readnb(s, 1)
    return !isopen(s) && nb_available(s)<=0
end

and also BufferStream discussion...

[samoconnor]

Re @quinnj comment about enforcing upload chunk size:

It seems like BufferStream could do this just by calling readbytes! to get the desired chunk size before writing it to the TCP socket. i.e. readbytes! will block until the requested number of bytes is available. If your concern with this is the extra copying of the chunk, perhaps we should look at adding something like takebytes!(::BufferStream, n).

The reason I'm concerned by this is that I can't see anything HTTP-specific about FIFOBuffer. If there are subtle behaviour differences with similar constructs in Base, the differences should probably be eliminated to avoid confusing behaviour. If there are no behaviour differences, and if FIFOBuffer has performance benefits, we should probably try to incorporate those benefits into Base.

julia> io = BufferStream()

julia> @async while true
           println(io, string(now())[1:19])
           sleep(1)
       end

julia> chunk_size = 10

julia> while !eof(io)
           chunk = Vector{UInt8}(chunk_size)
           readbytes!(io, chunk)
           println("upload_chunk: " * String(chunk))
       end
upload_chunk: 2017-08-23
upload_chunk: T09:45:14

upload_chunk: 2017-08-23
upload_chunk: T09:45:15

upload_chunk: 2017-08-23
upload_chunk: T09:45:16

...

[quinnj]

You seem so concerned w/ FIFOBuffer! You're right that there's nothing overly HTTP-specific about it. That was by design. I'd be happy to release it as a stand-alone package one day. Look, it fits the needs of HTTP really well. I've tried pretty hard to make sure the behavior is very predictable and close to constructs in Base. If there are specific issues, let's talk about them, but at this point, I personally don't have a lot of appetite to throw it all out and start over with something that's probably going to be harder to make work. If you're intent on not using FIFOBuffer, I'd glad review a PR that replaces it wholesale w/ BufferStream. I personally think it's quite the non-trivial task, so I'm going to spend my time on higher priorities.

[samoconnor]

OK, fair enough.
Specifically: #74 (comment)

[quinnj]

So do you think it'd be better if we never said eof until close was called explicitly?

[quinnj]

FWIW, I spent some time today trying out "eof only if close has been called". The main issue is it makes a lot of default cases harder to work with. For example, a lot of the time, users just want to pass in HTTP.post(url; body="cool body string", which gets wrapped in a FIFOBuffer underneath before making the actual request. If eof was only w/ close, we'd have to somehow keep track of whether we created the FIFOBuffer or if the user did; then we'd have to call close on it if we generated it w/ the request.

I did however, go with changing a default behavior so that when you do FIFOBuffer("cool body string"), it sets the eof field by default. This allows for the reading/writing of requests/responses to be nicely simplified and better match the behavior of Base.IOBuffer.

[samoconnor]

So do you think it'd be better if we never said eof until close was called explicitly?

Yes.

If there is a possibility that more data may be written to the FIFO, then eof must not return true.
eof should be "buffer empty" and "no possibility of future writes".
So, the only two cases I can see for eof returning true are if close() is called explicitly, or if it known a construction time that "this is a fixed buffer that will never be added to".

function FIFOBuffer(s::String)
    b = FIFOBuffer()
    write(b, s)
    close(b)
    return b
end

It seems to me there is no choice when implementing Base.eof(), but to follow the spec "If the stream is not yet exhausted, this function will block to wait for more data if necessary, and then return false."; and the [example in Base where eof is return !isopen(s) && nb_available(s)<=0] (https://github.com/JuliaLang/julia/blob/master/base/stream.jl#L1187).

There may be legitimate reasons not to follow the behaviour of Base.eof(), but in that case the function should have a different name. Perhaps nb_available ?.

[samoconnor]

For example, a lot of the time, users just want to pass in HTTP.post(url; body="cool body string", which gets wrapped in a FIFOBuffer underneath before making the actual request.

Why not just call close() immediately when you make the FIFOBuffer wrapper for the user's string?

[quinnj]

Alright, I'm definitely open to considering changing the definition of eof(f::FIFOBuffer) and requiring close to get an EOF. And I think it'll actually be much more possible now that some of the internal request/response reading/writing have been streamlined/simplified.

@omus, any thoughts here?

[omus]

I think requiring close could be a reasonable approach. Most of my issues with FIFOBuffer were have also occurred with getting early EOFs. As long as we don't run into cases where the FIFOBuffer is never closed this should work well.