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

pipeline into IOBuffer broken on Windows #39311

Closed
simeonschaub opened this issue Jan 18, 2021 · 10 comments · Fixed by #52461
Closed

pipeline into IOBuffer broken on Windows #39311

simeonschaub opened this issue Jan 18, 2021 · 10 comments · Fixed by #52461
Labels
bug Indicates an unexpected problem or unintended behavior io Involving the I/O subsystem: libuv, read, write, etc. system:windows Affects only Windows

Comments

@simeonschaub
Copy link
Member

simeonschaub commented Jan 18, 2021

MWE:

in a fresh session:

julia> sprint() do io
           run(pipeline(`echo foo`; stdout=io))
       end
""

julia> sprint() do io
           run(pipeline(`echo bar`; stdout=io))
       end
"bar\n"

I reduced this using a GitHub Actions Windows runner, but apparently this happens in Git Bash as well. I have seen this on master, as well as 1.5.

@simeonschaub simeonschaub added bug Indicates an unexpected problem or unintended behavior system:windows Affects only Windows labels Jan 18, 2021
simeonschaub added a commit to simeonschaub/FIFOStreams.jl that referenced this issue Jan 18, 2021
@vtjnash vtjnash added io Involving the I/O subsystem: libuv, read, write, etc. and removed system:windows Affects only Windows labels Jan 21, 2021
@vtjnash
Copy link
Member

vtjnash commented Jan 21, 2021

I think obviously that workaround is fundamentally flawed and should never be trusted. However, what's possibly less clear is whether we should remove the ability to set stdout to an IOBuffer, since it doesn't work correctly on any OS. The challenge is that since we're managing this buffer in Julia, we don't know to flush (or even close?) the buffer before the run command returns, so you can see old state.

@simeonschaub
Copy link
Member Author

Is there a proper way to pipe stdout to somewhere in memory on Windows? Could we allocate an unnamed pipe that's not managed by Julia, but which Julia could then read from without excessive copies? (Sorry if this is a dumb question, I am not too familiar with how exactly pipes are internally handled by the OS.) I haven't had any problems so far on Linux, but since you removed the Windows tag, is this something that may currently be broken on Unix as well?
It would be quite unfortunate if we had to remove the ability to pass IOBuffers as stdout without a good alternative, as I find this very useful in a lot of situations.

@vtjnash
Copy link
Member

vtjnash commented May 24, 2021

Yeah, Base.BufferStream (perhaps we should export that?)

@vtjnash
Copy link
Member

vtjnash commented May 24, 2021

And possibly (particularly after #39544), using open do instead of run pipeline.

@simeonschaub
Copy link
Member Author

Could you clarify how Base.BufferStream should be used properly in this instance? Does buffer_writes need to be set to true and do I need to take care of locking myself?

@vtjnash
Copy link
Member

vtjnash commented Jun 7, 2021

It is a drop-in replacement for IOBuffer in this case with the default constructor, etc.

@simeonschaub
Copy link
Member Author

Hmm, that does not seem to fix the issue: https://github.com/simeonschaub/FIFOStreams.jl/runs/2768490500

@vtjnash
Copy link
Member

vtjnash commented Jun 7, 2021

Violating the invariants of any object by inspecting a field won’t get you anything useful. You need to call ‘read’.

@simeonschaub
Copy link
Member Author

Ah, I was wondering about that. Is there a way to seek a BufferStream? Or would I need to start reading asynchronously before I start to write to the stream?

@vtjnash
Copy link
Member

vtjnash commented Jun 8, 2021

mark/reset should be defined, but the stream has flow control disabled by default so you don't need to read asynchronously

@ViralBShah ViralBShah added the system:windows Affects only Windows label Sep 5, 2022
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
bug Indicates an unexpected problem or unintended behavior io Involving the I/O subsystem: libuv, read, write, etc. system:windows Affects only Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants