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

Should Base.BufferStream have content present after eof(stream)==true? #49234

Closed
ericphanson opened this issue Apr 3, 2023 · 6 comments · Fixed by #52461
Closed

Should Base.BufferStream have content present after eof(stream)==true? #49234

ericphanson opened this issue Apr 3, 2023 · 6 comments · Fixed by #52461
Labels
io Involving the I/O subsystem: libuv, read, write, etc.

Comments

@ericphanson
Copy link
Contributor

ericphanson commented Apr 3, 2023

julia> io = Base.BufferStream()
BufferStream(bytes waiting=0, isopen=true)

julia> let
            t = @async begin
                for _ = 1:10
                    bytes = readavailable(io)
                    @show (String(bytes), eof(io), isopen(io))
                end
            end
            run(pipeline(`echo "hi"`, stdout=io))
            run(pipeline(`echo "bye"`, stdout=io))
        end
(String(bytes), eof(io), isopen(io)) = ("hi\n", true, false) # EOF is true here
(String(bytes), eof(io), isopen(io)) = ("bye\n", true, false) # we get more bytes out here
(String(bytes), eof(io), isopen(io)) = ("", true, false)
(String(bytes), eof(io), isopen(io)) = ("", true, false)
(String(bytes), eof(io), isopen(io)) = ("", true, false)
(String(bytes), eof(io), isopen(io)) = ("", true, false)
(String(bytes), eof(io), isopen(io)) = ("", true, false)
(String(bytes), eof(io), isopen(io)) = ("", true, false)
(String(bytes), eof(io), isopen(io)) = ("", true, false)
(String(bytes), eof(io), isopen(io)) = ("", true, false)
Process(`echo bye`, ProcessExited(0))

The docs for eof say:

Test whether an I/O stream is at end-of-file. If the stream is not yet exhausted, this function will block to wait for more data if necessary, and then return false. Therefore it is always safe to read one byte after seeing eof return false. eof will return false as long as buffered data is still available, even if the remote end of a connection is closed.

and that seems contradicted here, e.g. the stream does not seem "exhausted" but eof returns true. This example is related to #49233 (and JuliaLang/Pkg.jl#3430 (comment)) but Base.BufferStream seems to have particularly weird behavior in this case (compared to IOBuffer and Base.PipeBuffer) so I thought it was worth a separate issue.

xref #42424 which discusses other issues with Base.BufferStream

@vtjnash
Copy link
Member

vtjnash commented Apr 3, 2023

Sounds like a bug. The second run call should be throwing exceptions when it tries to operate on a closed object.

@giordano giordano added the io Involving the I/O subsystem: libuv, read, write, etc. label Apr 3, 2023
@pfitzseb
Copy link
Member

pfitzseb commented Apr 3, 2023

Why does run close the buffer? Is that documented somewhere?

@vtjnash
Copy link
Member

vtjnash commented Apr 3, 2023

It copies the underlying stream and is supposed to be "indistinguishable" from it. Since the underlying IO gets closed, so this gets closed too. If you didn't want that, then you should have made sure the underlying stream doesn't do that either. Which is to say, make a Pipe() yourself, and then explicitly close it when you want to indicate that it is done. There is some helper functions (e.g. Base.setup_stdio) that can assist with this.

@ericphanson
Copy link
Contributor Author

I’m trying to understand streams better-

It copies the underlying stream

here “it” is run?

the underlying IO gets closed

This is the underlying IO of the process being run?

make a Pipe() yourself

the difference here is Pipe is the Julia representation of an OS-level object, as opposed to a Julia object?

@pfitzseb
Copy link
Member

pfitzseb commented Apr 4, 2023

It copies the underlying stream and is supposed to be "indistinguishable" from it.

Fair enough, but that is not documented anywhere afaict. It totally makes sense that the fd or I/O stream would be closed by run, but an IOBuffer doesn't look like an object at the level of abstraction to me.

It's also worth noting that this changed somewhat recently (Julia 1.7 did not close the IOBuffer).

@vtjnash
Copy link
Member

vtjnash commented Apr 4, 2023

Note that IOBuffer should NEVER be used here for process spawn output, in any version of Julia, but I have not re-implemented the error for that yet.

vtjnash added a commit that referenced this issue Dec 8, 2023
People expect to use this (the docs even almost even suggested it at
some point), so it is better to make it work as expected (and better
than they can emulate) than to criticize their choices.

Also fix a few regressions and handling mistakes in setup_stdios:
 - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect
 - CmdRedirect did not allocate a ProcessChain, so it wouldd call
   setup_stdio then call setup_stdios on the result of that, which is
   strongly discouraged as setup_stdio(s) should only be called once
 - BufferStream was missing `check_open` calls before writing, and
   ignored `Base.reseteof` as a possible means of resuming writing after
   `closewrite` sends a shutdown message.

Fixes #39311
Fixes #49234
vtjnash added a commit that referenced this issue Dec 8, 2023
People expect to use this (the docs even almost even suggested it at
some point), so it is better to make it work as expected (and better
than they can emulate) than to criticize their choices.

Also fix a few regressions and handling mistakes in setup_stdios:
 - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect
 - CmdRedirect did not allocate a ProcessChain, so it wouldd call
   setup_stdio then call setup_stdios on the result of that, which is
   strongly discouraged as setup_stdio(s) should only be called once
 - BufferStream was missing `check_open` calls before writing, and
   ignored `Base.reseteof` as a possible means of resuming writing after
   `closewrite` sends a shutdown message.

Fixes #39311
Fixes #49234
Closes #49233
Closes #46768
vtjnash added a commit that referenced this issue Dec 8, 2023
People expect to use this (the docs even almost even suggested it at
some point), so it is better to make it work as expected (and better
than they can emulate) than to criticize their choices.

Also fix a few regressions and handling mistakes in setup_stdios:
 - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect
 - CmdRedirect did not allocate a ProcessChain, so it wouldd call
   setup_stdio then call setup_stdios on the result of that, which is
   strongly discouraged as setup_stdio(s) should only be called once
 - BufferStream was missing `check_open` calls before writing, and
   ignored `Base.reseteof` as a possible means of resuming writing after
   `closewrite` sends a shutdown message. Currently this fix is disabled
   because Pkg seems like a bit of a disaster with IO mismanagement.
 - Add `closewrite` to more methods, and document it.

Fixes #39311
Fixes #49234
Closes #49233
Closes #46768
vtjnash added a commit that referenced this issue Dec 8, 2023
People expect to use this (the docs even almost even suggested it at
some point), so it is better to make it work as expected (and better
than they can emulate) than to criticize their choices.

Also fix a few regressions and handling mistakes in setup_stdios:
 - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect
 - CmdRedirect did not allocate a ProcessChain, so it wouldd call
   setup_stdio then call setup_stdios on the result of that, which is
   strongly discouraged as setup_stdio(s) should only be called once
 - BufferStream was missing `check_open` calls before writing, and
   ignored `Base.reseteof` as a possible means of resuming writing after
   `closewrite` sends a shutdown message. Currently this fix is disabled
   because Pkg seems like a bit of a disaster with IO mismanagement.
 - Add `closewrite` to more methods, and document it.

Fixes #39311
Fixes #49234
Closes #49233
Closes #46768
vtjnash added a commit that referenced this issue Dec 9, 2023
People expect to use this (the docs even almost even suggested it at
some point), so it is better to make it work as expected (and better
than they can emulate) than to criticize their choices.

Also fix a few regressions and handling mistakes in setup_stdios:
 - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect
 - CmdRedirect did not allocate a ProcessChain, so it wouldd call
   setup_stdio then call setup_stdios on the result of that, which is
   strongly discouraged as setup_stdio(s) should only be called once
 - BufferStream was missing `check_open` calls before writing, and
   ignored `Base.reseteof` as a possible means of resuming writing after
   `closewrite` sends a shutdown message. Currently this fix is disabled
   because Pkg seems like a bit of a disaster with IO mismanagement.
 - Add `closewrite` to more methods, and document it.

Fixes #39311
Fixes #49234
Closes #49233
Closes #46768
vtjnash added a commit that referenced this issue Dec 11, 2023
People expect to use this (the docs even almost even suggested it at
some point), so it is better to make it work as expected (and better
than they can emulate) than to criticize their choices.

Also fix a few regressions and handling mistakes in setup_stdios:
 - #44500 tried to store a Redirectable into a SpawnIO, dropping FileRedirect
 - CmdRedirect did not allocate a ProcessChain, so it wouldd call
   setup_stdio then call setup_stdios on the result of that, which is
   strongly discouraged as setup_stdio(s) should only be called once
 - BufferStream was missing `check_open` calls before writing, and
   ignored `Base.reseteof` as a possible means of resuming writing after
   `closewrite` sends a shutdown message. Currently this fix is disabled
   because Pkg seems like a bit of a disaster with IO mismanagement.
 - Add `closewrite` to more methods, and document it.

Fixes #39311
Fixes #49234
Closes #49233
Closes #46768
vtjnash added a commit that referenced this issue Dec 13, 2023
People expect to use this (the docs even almost even suggested it at
some point), so it is better to make it work as expected (and better
than they can emulate) than to criticize their choices.

Also fix a few regressions and handling mistakes in setup_stdios:
- #44500 tried to store a Redirectable into a SpawnIO, dropping
FileRedirect
- CmdRedirect did not allocate a ProcessChain, so it would call
setup_stdio then call setup_stdios on the result of that, which is
strongly discouraged as setup_stdio(s) should only be called once
- BufferStream was missing `check_open` calls before writing, and
ignored `Base.reseteof` as a possible means of resuming writing after
`closewrite` sends a shutdown message.
 - Add `closewrite` to more methods, and document it.

Fixes #39311
Fixes #49234
Fixes #49233
Fixes #46768
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants