Open tpodowd opened 2 years ago
Hi @chzyer,
I noticed you became active again. Please have a look at this and see if you disagree with anything. I have tested the fixes on linux and windows 11 (used MS developer VM). I ran all the demo programs to check. I tried them with narrow and wide window sizes (although windows doesn't have code for window resize detection). Looking at the list of existing pull requests, I believe this pull request probably fixes some of the reported issues also. In particular, this PR replaces PR 171 (which I used to use but it was flawed).
Thanks!
@tpodowd Hey! great stuff, took your changes for a spin and experience lots of less redraws 👍 I try to maintain a fork of readline at https://github.com/wader/readline/tree/fq with various cherry-picked stuff and my own changes. Will let you know if i run into something strange.
Would be great if various forks of chzyer/readline could be merged into one somewhere for less conflicts and work. Best of course would be to get things into @chzyer repo if he agrees.
Thanks for trying out the PR @wader . Good to hear that it works for you so far. Do let me know if you find any issues. I'll also push back anything I find here but so far it is looking solid. @chzyer did a great job on this readline library so yes, it would be good to keep it all here.
Thanks @tpodowd , So many fixes in your PR, I will try them one by one.
@chzyer Great to see you active again and thanks a lot for the readline package, works very well and have been a joy to use
Hi @chzyer - sorry the PR is a bit large. I tried to keep it as focused as I could. I've been using the changes on linux for a while now and haven't noticed any issues so far. I've been talking to @wader separately and it seems they are using it successfully also on Windows which is promising :-)
Yeap i've been using fq daily with the patches on macOS for 2 weeks or so and haven't noticed any issues
Note: I rebased the screenredraw branch against the latest master and did a force push. No other changes.
This change appears to introduce a number of data races: https://gist.github.com/slingamn/fb0ed14d6a5471b83fce276d11f72856
Hi @slingamn - Thanks for reporting that. I'm a little swamped right now, but I'll try look at that this week. I was able to replicate by running.
go run -race readline-demo.go
» ==================
WARNING: DATA RACE
Read at 0x00c000090258 by goroutine 13:
github.com/chzyer/readline.(*RuneBuffer).getSplitByLine()
/home/tpodowd/work/readline/runebuf.go:449 +0x3c4
github.com/chzyer/readline.(*RuneBuffer).isInLineEdge()
/home/tpodowd/work/readline/runebuf.go:439 +0x8c
github.com/chzyer/readline.(*RuneBuffer).append()
/home/tpodowd/work/readline/runebuf.go:558 +0x20f
github.com/chzyer/readline.(*RuneBuffer).WriteRunes()
/home/tpodowd/work/readline/runebuf.go:175 +0x276
github.com/chzyer/readline.(*RuneBuffer).WriteRune()
/home/tpodowd/work/readline/runebuf.go:162 +0xa5a
github.com/chzyer/readline.(*Operation).ioloop()
/home/tpodowd/work/readline/operation.go:331 +0x9d1
Previous write at 0x00c000090258 by goroutine 15:
github.com/chzyer/readline.(*RuneBuffer).setOffset()
/home/tpodowd/work/readline/runebuf.go:525 +0x117
github.com/chzyer/readline.(*RuneBuffer).setOffset-fm()
/home/tpodowd/work/readline/runebuf.go:522 +0x5e
github.com/chzyer/readline.(*Terminal).GetOffset.func1()
/home/tpodowd/work/readline/terminal.go:81 +0x83
Goroutine 13 (running) created at:
github.com/chzyer/readline.NewOperation()
/home/tpodowd/work/readline/operation.go:88 +0x81a
github.com/chzyer/readline.(*Terminal).Readline()
/home/tpodowd/work/readline/terminal.go:95 +0x84
github.com/chzyer/readline.NewEx()
/home/tpodowd/work/readline/readline.go:169 +0x5f
main.main()
/home/tpodowd/work/readline/example/readline-demo/readline-demo.go:74 +0x270
Goroutine 15 (finished) created at:
github.com/chzyer/readline.(*Terminal).GetOffset()
/home/tpodowd/work/readline/terminal.go:80 +0x5a
github.com/chzyer/readline.(*RuneBuffer).getAndSetOffset()
/home/tpodowd/work/readline/runebuf.go:513 +0xe4
github.com/chzyer/readline.(*Operation).Runes()
/home/tpodowd/work/readline/operation.go:393 +0x1f6
github.com/chzyer/readline.(*Operation).String()
/home/tpodowd/work/readline/operation.go:376 +0x56
github.com/chzyer/readline.(*Instance).Readline()
/home/tpodowd/work/readline/readline.go:257 +0x2f
main.main()
/home/tpodowd/work/readline/example/readline-demo/readline-demo.go:99 +0x8af
==================
Thanks! Sorry, the full test case I was using is in the issue description of #217, in case that helps.
Given #198 and #219, I'm not sure it makes sense to "spot-fix" race conditions in this library. A large-scale concurrency redesign might be necessary (for example, putting most operations on internal datastructures behind a single broad mutex, releasing it for I/O operations).
@tpodowd @wader also interested in your thoughts on #220 if you have time
Hi @slingamn - Have not had a lot of time yet. Still high on my list. I did do some review though. the race condition above is a simple fix. There are a few more though so I think I'll just spend a little more time and address them also. It will be next week though as I'm off for a few days.
Note: I rebased this PR on the latest master and also added a small commit that fixes the race condition mentioned above that this PR introduced.
I am also going to rebase my other PR208 on top of this branch to include this race condition fix.
Thanks!
I tested this branch against the test case from #217. I could not reproduce the data race anymore. On the other hand:
> a
received a
> b
received b
> c
received c
> > d
received d
> > e
received e
> f
received f
> g
received g
I'm uncertain about the cause, but maybe it's the operation.go
changes in this branch?
Hi @slingamn. Just had a look at this. I think the patch to remove the IsReading check is probably not the right fix particularly combined with this PR.
So what is happening is that you have two go routines a and b. a) is calling readline and passing the string to b) before looping again and calling readline again. b) is looping printing the text a) sends it.
Everything works correctly if a) writes the prompt, reads the input and sends the input to b) which then prints the input and then a) writes the prompt again. However, if a) writes the prompt, reads input and sends input to b) and writes the prompt again before b) has time to write the input it received, you get the following:
> a
> received a
NEXTINPUTHERE
ie, the prompt "> " followed by "received a\n" If you simply type the "NEXTINPUTHERE" it goes at the start of the line as the prompt was already output and is now on the previous line.
If you press ctrl-a or something like that, the prompt will redraw as follows
> a
> received a
> NEXTINPUTHERE
The reason this happens is that this PR tries to avoid redrawing the whole line every time a key is pressed and simply appended to the current position.
I don't know if the library can/should fix "> received a" issue as it's simply a race writing to stdout? I can look into my fix for redrawing the prompt every append though which would at least mean that you will always get the prompt in the right place as you type without using ctrl-a or something to initiate the redraw. This would slow down every key press though (but I guess that is the way it is in the current mainline anyway). I'll play with it tomorrow if I have time. Let me know what you think.
Everything works correctly if a) writes the prompt, reads the input and sends the input to b) which then prints the input and then a) writes the prompt again.
As I understand the API here, it explicitly allows asynchronously writes via (*Instance).Write
while the prompt is displayed and input is being read. (Hence why (*wrapWriter).Write
explicitly supports redrawing the prompt, by wrapping the write in (*RuneBuffer).Refresh
.) The case for 7bb0e05674, i.e. removing the fast path that skips the refresh, is that this inherently involves a TOCTOU race, because IsReading()
can become true at any time.
Asynchronous writes are part of my core use case for this library (a rudimentary IRC client).
I don't know if the library can/should fix "> received a" issue as it's simply a race writing to stdout?
It seems to me that it's one of the core functions of this library to draw the prompt correctly. Drawing it incorrectly is not pleasant at a UX level (it makes the program feel inconsistent or unreliable). I don't know what's considered normal in TUI applications (this is my first time getting this close to terminal emulation) but it seems to me that any optimization that sacrifices correctness here isn't worth it. The performance penalty (which, as you point out, is present in the current master branch) doesn't seem to be affecting current consumers of this library (although I have noticed a weird "flicker" on Windows, possibly because of Windows idiosyncrasies).
Cool. Thanks for the feedback. I'll see what I can do.
Hi @slingamn
I pushed another commit to this PR. I believe that this addresses your issues. I removed the race condition with the prompt redraw. I removed the terminal isReading bool field as it is not reliable. Instead I introduced an operation.isPrompting bool with some locking around it so that it is reliable. This also conflicts with your patch to remove the shortcut https://github.com/chzyer/readline/issues/220 and fixes the prompt rewrite also such that you can now write to rl reliably and the prompt will not race. I also did not have to remove my append redraw optimisation I mentioned earlier as part of this fix so that was nice.
I ran though all the examples in the example directory also and insured that they worked (on linux).
Please try this out and let me know if things are behaving better for you. I'll try to get a windows VM up also to test it but this may take a little time. I also have not tested non-interactive mode yet.
Anyway. give it a go and let me know.
Hi @slingamn - I tested this branch also on a windows VM this morning. Looks good to me. I tested all the examples as well as your test program in #217, so I believe I haven't broken anything on windows.
@tpodowd thanks so much! I'm still testing, but I rebased my patchset https://github.com/slingamn/readline/commits/ircdog on your branch, and:
I ran into an issue with v1.5.1 where navigating the cursor over a long word-wrapped input line "moves the screen up". As if the library is trying to make sure that the prompt stays on the same line as the current cursor. Reproduced on macOS 14.0 with iTerm 3.4.21, TERM=xterm-256color.
This PR seems to fix that, so unless there are any other issues, I would recommend merging this PR.
In meantime, I am changing my application to point to this PR:
go mod edit -replace github.com/chzyer/readline@v1.5.1=github.com/cloudian/readline@screenredraw
go mod tidy
@Lekensteyn you might want to look at https://github.com/ergochat/readline which i think has these changes plus a bunch of more things