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

Add timeout to Base.prompt #39027

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Dec 29, 2020

Adds an optional timeout to prompt to return the default after a given number of seconds

See #39027 (comment)

@IanButterworth
Copy link
Member Author

IanButterworth commented Apr 6, 2021

I seem to be doing something illegal in the tests here by using redirect_stdin.

I just want to simulate prompt waiting on stdin for keypresses.

@test Base.prompt(IOButter(), buf, "baz", default="foobar", timeout = 1) == "foobar"

doesn't work because readline(IOBuffer()) returns empty immediately.

@test Base.prompt(stdin, buf, "baz", default="foobar", timeout = 1) == "foobar"

doesn't seem to work in the tests, perhaps because stdin reaches eof?

Is there another way to simulate an open stdin with no data?

@vtjnash
Copy link
Member

vtjnash commented Apr 6, 2021

throwto is an internal interface that may corrupt the task and stream state if used

@IanButterworth
Copy link
Member Author

I see. So implementing this timeout would need to be done at a lower read* level?

@IanButterworth
Copy link
Member Author

IanButterworth commented Apr 6, 2021

I thought about sending something to stdin to unblock readline. Is that possible? This approach doesn't work

function foo()
    Timer((t)->println(stdin), 1)
    readline(stdin)
end

@IanButterworth
Copy link
Member Author

IanButterworth commented Apr 13, 2021

Another approach that doesn't work. Fake closing the stdin.
It just hangs. The readline only seems to query whether the stdin is open once

begin
    stat_before = stdin.status
    Timer((t)->stdin.status = Base.StatusClosed, 4)
    try
        line = readline(stdin)
    finally 
        stdin.status = stat_before
    end
end

@vtjnash
Copy link
Member

vtjnash commented Apr 14, 2021

Oh yeah, I forget there was a fake StatusEOF that can be triggered manually. It should be okay to grab the correct locks and set that:

    lock(stream.cond)
    stream.status = StatusEOF
    notify(stream.cond)
    unlock(stream.cond)

It is likely not well-tested or supported, since we don't implement half-open streams properly, but it does exist already at least.

@IanButterworth
Copy link
Member Author

IanButterworth commented Apr 14, 2021

Nice! This works

julia> begin
           Timer(4) do t
               lock(stdin.cond)  
               stdin.status = Base.StatusEOF
               notify(stdin.cond)
               unlock(stdin.cond)
           end
           readline(stdin)
       end
""

julia>

base/util.jl Outdated Show resolved Hide resolved
@IanButterworth IanButterworth force-pushed the ib/prompt_timeout branch 2 times, most recently from 0af4206 to a23470c Compare April 14, 2021 18:27
@IanButterworth

This comment has been minimized.

@IanButterworth IanButterworth force-pushed the ib/prompt_timeout branch 2 times, most recently from 827f32b to 64b4b20 Compare April 15, 2021 23:18
@IanButterworth
Copy link
Member Author

@vtjnash Tests are finally passing. I took the lead from what other stdin tests do.

I introduced this as a second method to maintain the non-timeout method for IJulia etc.
And I couldn't figure out how to provide the same functionality for IJulia's stdin handler, which opens a text input box and seems quite unusual

image

@IanButterworth
Copy link
Member Author

Good to go @vtjnash ?

@jebej jebej mentioned this pull request Apr 20, 2021
@IanButterworth
Copy link
Member Author

Bump (this just came up on slack as a wanted feature)

@vtjnash vtjnash added the triage This should be discussed on a triage call label Apr 27, 2021
base/util.jl Outdated
For instance, the user enters a value and hits `return`:
```julia
julia> Base.prompt("Proceed? y/n"; default="n", timeout=5)
Proceed? y/n [n] timeout 5 seconds: y
Copy link
Member

Choose a reason for hiding this comment

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

I think the "timeout 5 seconds" part looks a bit strange or misplaced. Does it have to be displayed to the user at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems useful to have but probably a clearer wording like "Proceed? y/n. Default value [n] will be used in 5 seconds: y" would be better?

@JeffBezanson
Copy link
Member

This looks like a good feature but I'm iffy on the implementation. It would be much better not to define this only for LibuvStream, since if input is redirected you'll get a MethodError. It's also not great to poke at the LibuvStream internals. So I think this should wait until we have a reliable way to implement it.

@JeffBezanson
Copy link
Member

Good point from @vtjnash : arguably the timeout should be canceled as soon as you start typing.

@JeffBezanson
Copy link
Member

In that case, it's possible this can only really work for a TTY, and for other IO streams just ignore the timeout?

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jul 1, 2021
@IanButterworth IanButterworth marked this pull request as draft January 16, 2024 00:05
@IanButterworth
Copy link
Member Author

I've updated this with a focus on getting it to behave how it should

Except for the FIXME which I couldn't figure out, I think it's quite nice.

However the implementation needs review for properness.

Screen.Recording.2024-10-31.at.3.47.03.PM.mov

@IanButterworth IanButterworth marked this pull request as ready for review October 31, 2024 19:51
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.

5 participants