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.

Sign up for GitHub

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

Jump to bottom

Should run mark streams as not-writable after using them? #49233

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

Should run mark streams as not-writable after using them? #49233

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

Comments

@ericphanson
Copy link
Contributor 

julia> io=IOBuffer()
IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=1, mark=-1)

julia> iswritable(io)
true

julia> isopen(io)
true

julia> run(pipeline(`echo "hi"`, stdout=io))
Process(`echo hi`, ProcessExited(0))

julia> iswritable(io) # this is the interesting part
false

julia> isopen(io)
true

came up in JuliaLang/Pkg.jl#3430 (comment)
@ericphanson ericphanson mentioned this issue Apr 3, 2023
Closed
@vtjnash
Copy link
Member

vtjnash commented Apr 3, 2023

IOBuffer is supposed to be disallowed here entirely, but I keep being too busy to implement the error #46768

@ericphanson
Copy link
Contributor Author

in that case, could you comment in JuliaLang/Pkg.jl#3430 with how Pkg.test(; io) should work? (it internally calls run twice, directing stdout to io). It sounds like it is meant to support IOBuffer at least.

@giordano giordano added the io Involving the I/O subsystem: libuv, read, write, etc. label Apr 3, 2023
vtjnash added a commit that referenced this issue Dec 8, 2023
@vtjnash
spawn: permit using IOBuffer at stdout
28bbf92 
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
@vtjnash
spawn: permit using IOBuffer at stdout
2cad7d8 
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 vtjnash mentioned this issue Dec 8, 2023
Merged
vtjnash added a commit that referenced this issue Dec 8, 2023
@vtjnash
spawn: permit using IOBuffer at stdout
1f953d6 
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
@vtjnash
spawn: permit using IOBuffer at stdout
c516b7d 
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
@vtjnash
spawn: permit using IOBuffer at stdout
63059ae 
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
@vtjnash
spawn: permit using IOBuffer as stdout (#52461)
282e466 
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
Assignees
No one assigned
Labels
io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

spawn: permit using IOBuffer as stdout JuliaLang/julia
3 participants
@vtjnash @giordano @ericphanson