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

Fix fallback for errors in logging #3013

Merged
merged 3 commits into from
Sep 22, 2024
Merged

Fix fallback for errors in logging #3013

merged 3 commits into from
Sep 22, 2024

Conversation

danielwe
Copy link
Contributor

This fixes the bug identified in #3012, in which the catch block that's supposed to save you in case an error is thrown during logging ends up throwing an error of its own.

  • original_stderr is now assigned at runtime through the module's __init__ function, not at compile time as a hardcoded global constant. This was the source of the bug.
  • I rewrote the catch block's print/showerror as an @error log, which stands out more with the red frame and also looks more consistent with the other information logged to the REPL by Pluto.
  • Capturing stderr output for testing is not entirely straightforward. There are tradeoffs between using a local and remote worker. Please review the comments in the testing code and let me know which approach is preferred (or if there's a better design I haven't thought of).

Having looked more deeply at #3012, I think this PR is a sufficient fix on the Pluto side. The reason GPUCompiler's log messages error in the first place is that the purportedly safe logging macros are implemented incorrectly and aren't actually safe. I'll put together a PR to GPUCompiler for that.

Copy link
Contributor

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/danielwe/Pluto.jl", rev="loggingerror")
julia> using Pluto

@danielwe
Copy link
Contributor Author

Apparently, capturing stderr from the remote worker doesn't work on either Windows or julia 1.6; the latter because redirect_stderr(::Pipe) isn't supported in that version, the former for unknown reasons. I guess a local worker is the way to go for testing then. The latest commit implements that.

@fonsp
Copy link
Owner

fonsp commented Sep 17, 2024

Thanks for the research and the nice solution! Looks good!

(I changed the PR a bit to move the redirect_original_stderr functionality from the PlutoRunner source to the test.)

@fonsp fonsp linked an issue Sep 17, 2024 that may be closed by this pull request
@danielwe
Copy link
Contributor Author

The changes look good to me. Anything else you need before merging?

@fonsp fonsp merged commit 53e1981 into fonsp:main Sep 22, 2024
14 checks passed
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.

Error in fallback path for catching logging errors
2 participants