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

eof(::FileBuffer) incorrectly returns true before filling the buffer #134

Closed
iamed2 opened this issue Sep 10, 2021 · 2 comments · Fixed by #140
Closed

eof(::FileBuffer) incorrectly returns true before filling the buffer #134

iamed2 opened this issue Sep 10, 2021 · 2 comments · Fixed by #140

Comments

@iamed2
Copy link
Contributor

iamed2 commented Sep 10, 2021

According to the eof docs:

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

The current behaviour of always returning true before the first read means that the write(A::IO, B::IO) method (for B<:FileBuffer) and similar patterns are currently broken.

This is currently causing a problem for CSV.jl with S3Path and is also causing #126.

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Sep 27, 2021

Do you know how you'd like to see this fixed?

Do we want to read the file into the buffer before that check, e.g.

-Base.eof(buffer::FileBuffer) = eof(buffer.io)
+function Base.eof(buffer::FileBuffer)
+    if position(buffer) == 0
+        _read(buffer)
+        seekstart(buffer)
+    end
+    return eof(buffer.io)
+end

or do we want to avoid reading the file into the buffer and check the file directly like

-Base.eof(buffer::FileBuffer) = eof(buffer.io)
+function Base.eof(buffer::FileBuffer)
+    if position(buffer) == 0
+        return size(buffer.path) == 0
+    end
+    return eof(buffer.io)
+end

or is there another approach you had in mind?

@iamed2
Copy link
Contributor Author

iamed2 commented Sep 27, 2021

The former; the docs say following what I quoted above:

Therefore it is always safe to read one byte after seeing eof return false.

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 a pull request may close this issue.

2 participants