Skip to content

Conversation

@nhz2
Copy link
Member

@nhz2 nhz2 commented Feb 25, 2024

This fixes the issues with eof discussed in #168 (comment)

If eof returns false it is safe to read a byte. This means when the stream is in :write mode, eof must return true.

Also, if the stream is stopped when writing (stop_on_end=true) eof should not suddenly change from true to false. To fix this I added a :done state.

@nhz2 nhz2 marked this pull request as ready for review February 25, 2024 03:18
@nhz2 nhz2 requested a review from vtjnash February 25, 2024 03:18
if eof && sloweof(stream)
throw(EOFError())
if eof(stream)
ready_to_read!(stream)
Copy link
Contributor

@vtjnash vtjnash Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be implied by eof

Suggested change
ready_to_read!(stream)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means an EOFError is thrown when reading a byte from a closed stream.
This seems to match what happens with IOStream

julia> p, io = mktemp()
("/tmp/jl_GKa3GO", IOStream(<fd 21>))

julia> close(io)

julia> read(io, UInt8)
ERROR: EOFError: read end of file
Stacktrace:
 [1] read(s::IOStream, ::Type{UInt8})
   @ Base ./iostream.jl:400
 [2] top-level scope
   @ REPL[43]:1

But doesn't match with what IOBuffer does:

julia> stream = IOBuffer();

julia> close(stream)

julia> read(stream, UInt8)
ERROR: ArgumentError: read failed, IOBuffer is not readable
Stacktrace:
 [1] _throw_not_readable()
   @ Base ./iobuffer.jl:165
 [2] read(from::IOBuffer, ::Type{UInt8})
   @ Base ./iobuffer.jl:218
 [3] top-level scope
   @ REPL[49]:1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, though I think eof could optionally throw a state-exception in that case also sometimes. The ambiguity occurs when a stream is ambiguously readable or writable, which sometimes cannot be avoided. So seekable files usually close both ends at once (and eof is a transient condition, which changes to an error after calling close), whereas (non-seekable) streams the eof is a property of reading and isopen is a property of writing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm undoing the change because I like how IOBuffer is handling this better than IOStream.

For example, I think read(io, String) after close should error instead of returning an empty string:

julia> p, io = mktemp()

julia> close(io)

julia> read(io, String)
""
julia> stream = IOBuffer();

julia> close(stream)

julia> read(stream, String)
ERROR: ArgumentError: read failed, IOBuffer is not readable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that is already eof’s job to throw when there is a state violation, so it should not be necessary for read to also redundantly check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break this test that was added in #32:

stream = NoopStream(IOBuffer(""))
unsafe_write(stream, C_NULL, 0)
@test eof(stream) # write
close(stream)
@test eof(stream) # close

Copy link
Member Author

@nhz2 nhz2 Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that eof should throw if the stream is not in a readable state.

However this seems like a breaking change, so I will split this PR into smaller parts, and add the eof changes in v0.11 for v0.11 I would also like to fix #93, #109 and remove the seekend implementation.

@nhz2 nhz2 marked this pull request as draft February 27, 2024 15:50
@nhz2 nhz2 mentioned this pull request Mar 6, 2024
@nhz2 nhz2 closed this Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants