Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

close(::GenericIOBuffer) has a bug and is too aggressive in some cases #38703

Open
kwdye opened this issue Dec 4, 2020 · 4 comments
Open

close(::GenericIOBuffer) has a bug and is too aggressive in some cases #38703

kwdye opened this issue Dec 4, 2020 · 4 comments
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc.

Comments

@kwdye
Copy link

kwdye commented Dec 4, 2020

I encountered what appears to be a small bug in close(::GenericIOBuffer). Here is the code from v1.5.3

@noinline function close(io::Base.GenericIOBuffer{T}) where T
    io.readable = false
    io.writable = false
    io.seekable = false
    io.size = 0
    io.maxsize = 0
    io.ptr = 1
    io.mark = -1
    if io.writable
        resize!(io.data, 0)
    end
    nothing
end

The first issue is just that the "if io.writable" condition can never be true because writable is set to false earlier in the function. I don't understand why we should even want to resize the data to zero in any case though since you can optionally provide this data yourself in the IOBuffer constructor. I think most users would find their data array being resized to zero to be an unhelpful surprise.

The second issue which is more important is that close() is very aggressive about resetting the size and ptr fields, making the data effectively inaccessible. This causes difficulties when using GenericIOBuffer as a stream that's been wrapped by another stream ie:

data = zeros(UInt8,80)
iob = IOBuffer(data)
seek(iob,0)
compressor = TranscodingStream(ZstdCompressor(), iob)

The issue is that compressor won't necessarily have written all of its output to the IOBuffer until it has been closed, and closing it calls close on the IOBuffer, rendering the data inaccessible (besides manually inspecting the data field). There is no way to tell how many bytes were actually written to data, unless data was extended in size because it was too small.

I'm not sure if there would be any bad effects to making close(::GenericIOBuffer) less aggressive, perhaps only setting writable to false and otherwise doing nothing. It seems like this is a case where the behavior you expect from a buffer and the behavior you expect from a stream diverge.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 4, 2020

Perhaps you're looking for a Base.BufferStream?

@kwdye
Copy link
Author

kwdye commented Dec 4, 2020

Yes thanks BufferStream looks like what I'd want here. Is there a reason it isn't exported?

@StefanKarpinski
Copy link
Sponsor Member

In my experience Base.BufferStream is quite buggy. You may want to use https://github.com/staticfloat/SimpleBufferStream.jl instead. Perhaps @staticfloat can elaborate on why Base.BufferStream didn't do what he needed (I can't recall).

@staticfloat
Copy link
Sponsor Member

I don't remember what the exact issue was; it may have been as simple as that Base.BufferStream doesn't inherit from the right abstract interface for usage with HTTP. In any case, SimpleBufferStream does work pretty well for what it is (it's not fully-featured, e.g. there's no mark()/unmark() interface).

@kshyatt kshyatt added the domain:io Involving the I/O subsystem: libuv, read, write, etc. label Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Development

No branches or pull requests

5 participants