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.

Sign up for GitHub

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

SnoopPrecompile + pipe handling #344

Merged
merged 4 commits into from
Feb 3, 2023
Merged

SnoopPrecompile + pipe handling #344

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43f7689
SnoopPrecompile + pipe handling
timholy Jan 22, 2023
dd874b9
Add an async reader
timholy Jan 25, 2023
ab6f1a8
Fix pipe-handling
timholy Jan 26, 2023
ab7a6a9
Simpler version
timholy Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/Cthulhu.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,9 +818,14 @@ using SnoopPrecompile
descend(gcd, (Int, Int); terminal=term)
readuntil(output.out, "↩")
end
close(input.in)
close(output.in)
close(err.in)
@sync begin
@async read(output.out, String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line needs to be listed before you start writing to the pipe. I can't suggest on a large enough region in github, but roughly:

input, output, err = linked_pipe(), linked_pipe(), linked_pipe()
# set up reader async support
tout = @async write(devnull, output)
terr = @async write(devnull, err) # or read(devnull, String) if you want error messages
# do work here
write(input.in, 'q')
@precompile_all_calls begin
    descend(gcd, (Int, Int); terminal=term)
    # declare we are done with streams
    close(input.in)
    close(output.in)
    close(err.in)
    # pause for the REPL to finish precompiling
    wait(tout)
    wait(terr)
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(you could also still use @sync instead of the explicit variables with wait—the macros expand to the same thing, but it seems it would be harder to nest precompile_all_calls in that case)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(very strictly, the write(input.in, 'q') should also be @async too, but kernel buffering doesn't care that much about a single byte. The rules guidelines are that (a) each Task can manage any number of in objects (such as stdout/stderr), but should only access exactly one out (such as stdin) and (b) each end of an IO object should be accessed only from one Task. The second rule means that if we moved the 'q' into a separate task, the close(input.in) call must go with it too into the same task.)

Sorry, I don't know any way currently to help simplify this so that IO is harder to mess up. It is just part of the reality of writing async-event-driven programs. For something like linked_pipe, perhaps Base could have a callback interface for this, so that there is a visible separation between the object behaviors like this:

in, t = linked_pipe() do out
    # what to do with output here
    @show read(out, String)
end
# what to do with input end here
write(in, "Hello\n")
close(in)
wait(t)

Where linked_pipe is pretty simple:

function linked_pipe(reader::Callable)
    (; in, out) = linked_pipe()
    in, @async reader(out)
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, define that linked_pipe() = Base.BufferStream(). Since that is an immediate Julia object without kernel backing and with back-pressure disabled, it lets you write pretty straightforward code that would be incorrect for a general IO object, but works as desired without to manage Tasks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a nice idea, except

julia> descend(gcd, (Int, Int); terminal=term)
┌ Warning: TerminalMenus: Unable to enter raw mode:
│   exception =
│    type BufferStream has no field handle
│    Stacktrace:
│      [1] getproperty(stream::Base.BufferStream, name::Symbol)
│        @ Base ./stream.jl:60
│      [2] raw!(t::REPL.Terminals.TTYTerminal, raw::Bool)
│        @ REPL.Terminals ~/.julia/juliaup/julia-1.9.0-beta3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/Terminals.jl:139
│      [3] request(term::REPL.Terminals.TTYTerminal, m::Cthulhu.CthulhuMenu; cursor::Int64, suppress_output::Bool)
│        @ REPL.TerminalMenus ~/.julia/juliaup/julia-1.9.0-beta3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/TerminalMenus/AbstractMenu.jl:192
│      [4] request
│        @ ~/.julia/juliaup/julia-1.9.0-beta3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/TerminalMenus/AbstractMenu.jl:181 [inlined]
│      [5] request(term::REPL.Terminals.TTYTerminal, msg::String, m::Cthulhu.CthulhuMenu; kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
│        @ REPL.TerminalMenus ~/.julia/juliaup/julia-1.9.0-beta3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/TerminalMenus/AbstractMenu.jl:259
│      [6] request(term::REPL.Terminals.TTYTerminal, msg::String, m::Cthulhu.CthulhuMenu)
│        @ REPL.TerminalMenus ~/.julia/juliaup/julia-1.9.0-beta3+0.x64.linux.gnu/share/julia/stdlib/v1.9/REPL/src/TerminalMenus/AbstractMenu.jl:257
│      [7] _descend(term::REPL.Terminals.TTYTerminal, interp::Cthulhu.CthulhuInterpreter, curs::Cthulhu.CthulhuCursor; override::Nothing, debuginfo::Symbol, optimize::Bool, interruptexc::Bool, iswarn::Bool, hide_type_stable::Bool, verbose::Nothing, remarks::Bool, with_effects::Bool, inline_cost::Bool, type_annotations::Bool)
│        @ Cthulhu ~/.julia/dev/Cthulhu/src/Cthulhu.jl:538

Would it be more appropriate to add a check to raw! or should we not use a BufferedStream?

@async begin
close(input.in)
close(output.in)
close(err.in)
end
end
nothing
end

Expand Down