Experiment: replacing FIFOBuffer with BufferStream/IOBuffer

[samoconnor]

Following on from previous discussion of FIFOBuffer vs BufferStream #74 (comment):

I'd [be] glad review a PR that replaces it wholesale w/ BufferStream ...

@quinnj, as you know, I have a preference for using standard Base buffer types with HTTP rather than introducing the special purpose FIFOBuffer type.

Irrespective wether a decision is eventually made to replace FIFOBuffer with BufferStream or similar, I figure that it is a worthwhile exercise to try a quick hack that makes FIFOBuffer a very thin wrapper for IOBuffer/BufferStream, and see what we learn. e.g. we might find bugs, or behaviour inconsistencies in FIFOBuffer, or in BufferStream or IOBuffer etc.

A replacement FIFOBuffer that simply wraps IOBuffer/BufferStream is here.

Tweaks to the rest of HTTP.jl are on this branch: samoconnor@d572554

Results so far...

All the HTTP.jl tests are passing. The tests for my AWS packages also pass using this version of HTTP.jl.

I've found one new bug in Base: JuliaLang/julia#24465

I'm reminded of other inconsistencies in Base:

• RFC wait() support for GenericIOBuffer/PipeBuffer JuliaLang/julia#23391 (comment)
• nb_available(open(fn)) != nb_available(Filesystem.open(fn)) JuliaLang/julia#14624

I noticed that the original read(::FIFOBuffer) was non-blocking. In my branch it is blocking (because it just wraps the Base function, so I've changed some uses of read() to readavailable() where non-blocking behaviour is needed (but, see this: JuliaLang/julia#16821).

The old FIFOBuffer eof() function never blocked. The wrapped Base.eof() does block as per the spec: "If the stream is not yet exhausted, this function will block to wait for more data if necessary, and then return false.". So, there are places were I replaced an eof() call with a nb_available() check or a comparison of bytes received with Content-Length.

The branch removes the fixed size buffer option from FIFOBuffer (because the wrapped BufferStream hides the buffer size and manages it internally). This means that the test/fifobuffer.jl test cases that probe the filling up of the fixed buffer are essentially now just hacked to pass. I don't think I properly understand the use case you had in mind for the fixed size feature. In all my usage it seems more convenient to let the buffer auto-size itself as needed. However, the underlying IOBuffer used by Base's BufferStream supports having a fixed size, so it should be easy enough to make a PR that adds a fixed size option to BufferStream if that is needed.

The String(::FIFOBuffer) function is useful. Base doesn't have a way to non-destructively read from (peek at?) an IO buffer, other than using mark/reset. I'm not sure if it would be better to call this peak(::FIFOBuffer, String). The name of the function would have to make sense for all the other IO types (including local files, TCP, TTY, etc).

Given that this experimental FIFOBuffer wrapping BufferStream/IOBuffer seems to work, it shouldn't be too hard to go the next step and replace FIFOBuffer with Base types.

[quinnj]

As you can probably tell, I'm not in any rush to throw FIFOBuffer out. As most of Base IO stuff will probably not be completely set in stone, even maybe for 1.0, I foresee FIFOBuffer staying as is for at least the foreseeable future, if anything as a decoupled-from-Base experiment in how Base IO interfaces could improve.