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

Redirect stdout to devnull during precompilation #231

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

bauglir
Copy link
Contributor

@bauglir bauglir commented Nov 27, 2023

Calling redirect_stdout without any arguments redirects the output to a newly created Pipe1 (which is mostly a wrapper around standard *nix pipes). Not reading data from this Pipe can result in precompilation to hang, e.g. in CI environments such as GitHub Actions (as was the case for me) or as reported here2.

This is due to a fundamental expectation that a (*nix) kernel has on pipes, as documented3, "write will block until sufficient data has been read from the pipe to allow the write to complete". For this reason never reading from new_stdout may cause the precompilation to hang. Explicitly redirecting to devnull circumvents this expectation and ensures precompilation succeeds.

I was able to verify the bug and the fix due to our (internal) CI systems getting stuck and trying with and without the fix (a couple of times to ensure the test was reliable/reproducible). Unfortunately, I don't have any publicly available logs to show as I couldn't get this reproduced on standard GitHub Actions runners. Likely because these always start with a fresh Julia depot and it appears that every time this becomes an issue some problem with the depot seems to trigger it.

Thanks to @vtjnash for pointing me in the right direction.

Footnotes

  1. https://github.com/JuliaLang/julia/blob/9233a16172139c06107b475480bfce098f10a8a6/base/stream.jl#L1259

  2. https://discourse.julialang.org/t/when-i-add-any-package-prettytables-will-get-stuck-here-and-cannot-be-executed-i-need-to-control-c-to-close-it

  3. https://man7.org/linux/man-pages/man7/pipe.7.html

Calling `redirect_stdout` without any arguments redirects the output to
a newly created `Pipe`[^1] (which is mostly a wrapper around standard
*nix pipes). Not reading data from this `Pipe` can result in
precompilation to hang, e.g. in CI environments such as GitHub Actions
or as reported here[^2].

This is due to a fundamental expectation that a (*nix) kernel has on
pipes, as documented[^3] "write will block until sufficient data has
been read from the pipe to allow the write to complete". For this reason
never reading from `new_stdout` may cause the precompilation to hang.
Explicitly redirecting to `devnull` circumvents this expectation and
ensures precompilation succeeds.

[^1]: https://github.com/JuliaLang/julia/blob/9233a16172139c06107b475480bfce098f10a8a6/base/stream.jl#L1259
[^2]: https://discourse.julialang.org/t/when-i-add-any-package-prettytables-will-get-stuck-here-and-cannot-be-executed-i-need-to-control-c-to-close-it
[^3]: https://man7.org/linux/man-pages/man7/pipe.7.html
@KristofferC
Copy link

What's the reason for redirecting stdout at all intead of just passing an IO to the functions below?

@ronisbr
Copy link
Owner

ronisbr commented Nov 28, 2023

Hi @bauglir !

Thanks for the very detailed report!

What's the reason for redirecting stdout at all intead of just passing an IO to the functions below?

Hi @KristofferC!

When I created the precompile statements using SnoopCompile.jl sometime ago, the time to print the first table was significantly reduced if I used those calls, which set io = stdout. I guessed that, by using IOBuffer, the system was compiling different versions of the functions compared to what is called in the REPL, when the IO is the stdout. Hence, I kept the same behavior even when I switched to PrecompilationTools.jl. However, in this case, I needed to redirect the stdout so that the user does not see a lot of output when the package gets precompiled.

Do you think that this behavior was improved in recent Julia versions and the approach deserves revisiting?

@ronisbr ronisbr merged commit 8cf53ff into ronisbr:master Nov 28, 2023
13 of 14 checks passed
@bauglir bauglir deleted the fix-precompilation-hang branch November 28, 2023 13:21
@bauglir
Copy link
Contributor Author

bauglir commented Nov 28, 2023

Thanks for merging and getting the release out!

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.

3 participants