Catch failure to open Worker and write out port

[jkrumbiegel]

We encountered a failure on CI where starting a Malt worker resulted in an obscure error. Something like:

julia> worker = Malt.Worker(; exeflags = ["-t invalid"])

ERROR: ArgumentError: input string is empty or only contains whitespace
Stacktrace:
 [1] tryparse_internal(::Type{UInt16}, s::String, startpos::Int64, endpos::Int64, base_::Int64, raise::Bool)
   @ Base ./parse.jl:115
 [2] parse(::Type{UInt16}, s::String; base::Nothing)
   @ Base ./parse.jl:254
 [3] parse
   @ ./parse.jl:253 [inlined]
 [4] Malt.Worker(; env::Vector{String}, exeflags::Vector{String})
   @ Malt ~/.julia/dev/Malt/src/Malt.jl:118
 [5] top-level scope

Turns out when a worker crashes before a port is written back, stdout is read back as "" so you get an error when parsing that as the port number. But the underlying reason is not visible.

I tried here to read from stderr to report back what happened if reading the port fails. I didn't know off the top of my head how to do this more elegantly. I'm also not sure if proc could stall if the worker manages to start up correctly but then writes stuff there over time but nobody reads the pipe.

With this change you get this behavior on crashes:

julia> worker = Malt.Worker(; exeflags = ["-t invalid"])
ERROR: Failed to start worker process correctly. Expected to read port from stdout, got "" instead. Stderr output was:
ERROR: julia: -t,--threads=<n>[,auto|<m>]; n must be an integer >= 1

Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] Malt.Worker(; env::Vector{String}, exeflags::Vector{String})
   @ Malt ~/.julia/dev/Malt/src/Malt.jl:131
 [3] top-level scope
   @ REPL[34]:1

cc @MichaelHatherly

[fonsp]

Why not just err_output = read(err, String)? Can you add a comment?

[jkrumbiegel]

Good question, I think this was an artifact of me moving things around. It doesn't make sense this way anymore.

[fonsp]

Thanks! Sorry for the delay, I took a small break :)

My hesitation is what happens when the worker writes to stderr during normal use (after launch). I can't remember where this currently goes, but we need to make sure in this PR that this is unaffected.

If stderr is currently ignored, then this PR is actually a good opportunity to fix that as well :)

[fonsp]

Ah there is #82 about this

[fonsp]

Before this PR

julia> Malt.remote_eval_fetch(worker, :(println(stdout, 123)))

julia> Malt.remote_eval_fetch(worker, :(println(stderr, 123)))
123

After this PR

julia> Malt.remote_eval_fetch(worker, :(println(stdout, 123)))

julia> Malt.remote_eval_fetch(worker, :(println(stderr, 123)))

It looks like both implementations are incorrect/undesirable, see #82. Let's implement this PR together with fixing #82.

[fonsp]

I wonder how to do that, first capture a process's stdout to a buffer (to read the port on stdout and launch error on stderr), and the switching to redirecting it to stdout).

EDIT: looks like Distributed does it with @async read write: #82 (comment) . That shouldn't be too hard to implement!

[svilupp]

This PR is a great idea! We've had several of these failures today and are not sure why it started happening:

PumasAI/QuartoNotebookRunner.jl#223