From 050860a56df1e6adb4f9e8712be72e529a8374df Mon Sep 17 00:00:00 2001 From: Daniel Wennberg Date: Mon, 26 Aug 2024 13:13:53 -0700 Subject: [PATCH] Fix fallback for errors in logging --- src/runner/PlutoRunner/src/PlutoRunner.jl | 2 + .../src/evaluation/deleting globals.jl | 4 +- src/runner/PlutoRunner/src/io/logging.jl | 23 +++++-- test/Logging.jl | 61 +++++++++++++++++++ 4 files changed, 84 insertions(+), 6 deletions(-) diff --git a/src/runner/PlutoRunner/src/PlutoRunner.jl b/src/runner/PlutoRunner/src/PlutoRunner.jl index 37d6184a07..13efe1e4ed 100644 --- a/src/runner/PlutoRunner/src/PlutoRunner.jl +++ b/src/runner/PlutoRunner/src/PlutoRunner.jl @@ -76,4 +76,6 @@ include("./io/logging.jl") include("./io/stdout.jl") include("./precompile.jl") +__init__() = redirect_original_stderr(stderr) + end diff --git a/src/runner/PlutoRunner/src/evaluation/deleting globals.jl b/src/runner/PlutoRunner/src/evaluation/deleting globals.jl index b918d80fe8..94881905f8 100644 --- a/src/runner/PlutoRunner/src/evaluation/deleting globals.jl +++ b/src/runner/PlutoRunner/src/evaluation/deleting globals.jl @@ -89,7 +89,7 @@ function move_vars( catch ex if !(ex isa UndefVarError) @warn "Failed to move variable $(symbol) to new workspace:" - showerror(original_stderr, ex, stacktrace(catch_backtrace())) + showerror(original_stderr[], ex, stacktrace(catch_backtrace())) end end end @@ -198,7 +198,7 @@ function try_delete_toplevel_methods(workspace::Module, (cell_id, name_parts)::T (val isa Function) && delete_toplevel_methods(val, cell_id) catch ex @warn "Failed to delete methods for $(name_parts)" - showerror(original_stderr, ex, stacktrace(catch_backtrace())) + showerror(original_stderr[], ex, stacktrace(catch_backtrace())) false end catch diff --git a/src/runner/PlutoRunner/src/io/logging.jl b/src/runner/PlutoRunner/src/io/logging.jl index e1d3d05072..4522e246b5 100644 --- a/src/runner/PlutoRunner/src/io/logging.jl +++ b/src/runner/PlutoRunner/src/io/logging.jl @@ -1,7 +1,21 @@ import Logging -const original_stdout = stdout -const original_stderr = stderr +const original_stderr = Ref{IO}() + +function redirect_original_stderr(io) + original_stderr[] = io + return nothing +end + +function redirect_original_stderr(f::Function, io) + old_stderr = original_stderr[] + redirect_original_stderr(io) + try + return f() + finally + redirect_original_stderr(old_stderr) + end +end const old_logger = Ref{Union{Logging.AbstractLogger,Nothing}}(nothing) @@ -113,8 +127,9 @@ function Logging.handle_message(pl::PlutoCellLogger, level, msg, _module, group, yield() catch e - println(original_stderr, "Failed to relay log from PlutoRunner") - showerror(original_stderr, e, stacktrace(catch_backtrace())) + Logging.with_logger(Logging.ConsoleLogger(original_stderr[])) do + @error "Failed to relay log from PlutoRunner" exception=(e, catch_backtrace()) + end nothing end diff --git a/test/Logging.jl b/test/Logging.jl index c34d3f2654..3f2ba8243f 100644 --- a/test/Logging.jl +++ b/test/Logging.jl @@ -66,6 +66,14 @@ using Pluto.WorkspaceManager: poll "show(stdout, collect(1:500))", # 24 "show(stdout, \"text/plain\", collect(1:500))", # 25 "display(collect(1:500))", # 26 + + "struct StructThatErrorsOnPrinting end", # 27 + """ + Base.print(::IO, ::StructThatErrorsOnPrinting) = error("Can't print this") + """, # 28 + """ + @info "" _id=StructThatErrorsOnPrinting() + """, # 29 ])) @testset "Stdout" begin @@ -201,4 +209,57 @@ using Pluto.WorkspaceManager: poll end cleanup(🍭, notebook) + + @testset "Logging error fallback" begin + # This testset needs a separate server session to capture the process stderr (which + # is different from the notebook stderr) + 🍍 = ServerSession() + + msg = if true + # Alternative 1: use a remote worker and capture stderr using redirect_stderr. + # The reason we cannot use the same session as above is that a Malt.Worker's + # stderr is bound to the local stderr when the worker is instantiated, which + # happens at the first call to update_run! with a given session and notebook. + # Since update_run! has been called several times above, the existing worker's + # stderr is already bound to the local TTY, and redirect_stderr won't change + # that. We need a new session such that we can instantiate a new worker within + # the redirect_stderr block. (An alternative would be to move this testset to + # the top of the file such that it gets the first call to update_run!, but it + # seems like a bad idea to have tests that may pass or fail depending on the + # order of the testsets.) + # + # This argument is specific to Malt. Distributed relays stderr differently. + 🍍.options.evaluation.workspace_use_distributed = true + + pipe = Pipe() + redirect_stderr(pipe) do + update_run!(🍍, notebook, notebook.cells[27:29]) + end + t = Threads.@spawn read(pipe, String) + close(pipe) + fetch(t) + else + # Alternative 2: use a local worker and capture stderr using + # PlutoRunner.redirect_original_stderr. This avoids the complications due to + # stderr relaying between processes and Malt implementation details, but + # requires using a bespoke redirection function from PlutoRunner rather than + # Base.redirect_stderr. + 🍍.options.evaluation.workspace_use_distributed = false + + io = IOBuffer() + PlutoRunner.redirect_original_stderr(io) do + update_run!(🍍, notebook, notebook.cells[27:29]) + end + msg_ = String(take!(io)) + close(io) + msg_ + end + + @test notebook.cells[27] |> noerror + @test notebook.cells[28] |> noerror + @test notebook.cells[29] |> noerror + @test occursin("Failed to relay log from PlutoRunner", msg) + + cleanup(🍍, notebook) + end end