Skip to content

Commit

Permalink
Fix terminal resize race conditions
Browse files Browse the repository at this point in the history 
The runebuf, search and complete data structures all maintain their
own cache of the terminal width and height. When the terminal is
resized, a signal is sent to a go routine which calls a function
that sets the new width and height to each data structure instance.
However, this races with the main thread reading the sizes.

Instead of introducing more locks, it makes sense that the terminal
itself caches it's width and height and the other structures just
get it as necessary. This removes all the racing.

As part of this change, search, complete and runebuf constructor
changes to no longer require the initial sizes. Also each structure
needs a reference to the Terminal so they can get the width/height.
As the io.Writer parameter is actually the terminal anyway, the
simpliest option was just to change the type from the io.Writer to
Terminal. I don't believe that anyone would be calling these
functions directly so the signature changes should be ok. I also
removed the no longer used OnWidthChange() and OnSizeChange()
functions from these three structures.
  • Loading branch information
@tpodowd
tpodowd committed Feb 28, 2023
1 parent 4fda9f0 commit f0ae515
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 96 deletions.
57 changes: 25 additions & 32 deletions complete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bufio"
"bytes"
"fmt"
"io"
)

type AutoCompleter interface {
Expand All @@ -25,10 +24,8 @@ func (t *TabCompleter) Do([]rune, int) ([][]rune, int) {
}

type opCompleter struct {
w io.Writer
w *Terminal
op *Operation
width int
height int

inCompleteMode bool
inSelectMode bool
Expand All @@ -42,12 +39,10 @@ type opCompleter struct {
candidateColWidth int // width of candidate columns
}

func newOpCompleter(w io.Writer, op *Operation, width, height int) *opCompleter {
func newOpCompleter(w *Terminal, op *Operation) *opCompleter {
return &opCompleter{
w: w,
op: op,
width: width,
height: height,
}
}

Expand All @@ -73,7 +68,8 @@ func (o *opCompleter) nextCandidate(i int) {
// when tab pressed if cannot do complete for reason such as width unknown
// or no candidates available.
func (o *opCompleter) OnComplete() (ringBell bool) {
if o.width == 0 || o.height < 3 {
tWidth, tHeight := o.w.GetWidthHeight()
if tWidth == 0 || tHeight < 3 {
return false
}
if o.IsInCompleteSelectMode() {
Expand Down Expand Up @@ -103,7 +99,7 @@ func (o *opCompleter) OnComplete() (ringBell bool) {
if len(newLines) == 0 || (len(newLines) == 1 && len(newLines[0]) == 0) {
o.ExitCompleteMode(false)
return false // will ring bell on initial tab press
}
}
if o.candidateOff > offset {
// part of buffer we are completing has changed. Example might be that we were completing "ls" and
// user typed space so we are no longer completing "ls" but now we are completing an argument of
Expand Down Expand Up @@ -246,15 +242,6 @@ func (o *opCompleter) getMatrixSize() int {
return line * colNum
}

func (o *opCompleter) OnWidthChange(newWidth int) {
o.width = newWidth
}

func (o *opCompleter) OnSizeChange(newWidth, newHeight int) {
o.width = newWidth
o.height = newHeight
}

// setColumnInfo calculates column width and number of columns required
// to present the list of candidates on the terminal.
func (o *opCompleter) setColumnInfo() {
Expand All @@ -270,8 +257,10 @@ func (o *opCompleter) setColumnInfo() {
}
colWidth++ // whitespace between cols

tWidth, _ := o.w.GetWidthHeight()

// -1 to avoid end of line issues
width := o.width - 1
width := tWidth - 1
colNum := width / colWidth
if colNum != 0 {
colWidth += (width - (colWidth * colNum)) / colNum
Expand All @@ -283,8 +272,9 @@ func (o *opCompleter) setColumnInfo() {

// needPagerMode returns true if number of candidates would go off the page
func (o *opCompleter) needPagerMode() bool {
tWidth, tHeight := o.w.GetWidthHeight()
buflineCnt := o.op.buf.LineCount() // lines taken by buffer content
linesAvail := o.height - buflineCnt // lines available without scrolling buffer off screen
linesAvail := tHeight - buflineCnt // lines available without scrolling buffer off screen
if o.candidateColNum > 0 {
// Normal case where each candidate at least fits on a line
maxOrPage := linesAvail * o.candidateColNum // max candiates without needing to page
Expand All @@ -299,9 +289,9 @@ func (o *opCompleter) needPagerMode() bool {
for _, c := range o.candidate {
cWidth := sameWidth + runes.WidthAll(c)
cLines := 1
if o.width > 0 {
cLines = cWidth / o.width
if cWidth % o.width > 0 {
if tWidth > 0 {
cLines = cWidth / tWidth
if cWidth % tWidth > 0 {
cLines++
}
}
Expand All @@ -326,6 +316,7 @@ func (o *opCompleter) CompleteRefresh() {
buf.WriteString("\033[J")

same := o.op.buf.RuneSlice(-o.candidateOff)
tWidth, _ := o.w.GetWidthHeight()

colIdx := 0
lines := 0
Expand All @@ -334,13 +325,13 @@ func (o *opCompleter) CompleteRefresh() {
inSelect := idx == o.candidateChoise && o.IsInCompleteSelectMode()
cWidth := sameWidth + runes.WidthAll(c)
cLines := 1
if o.width > 0 {
if tWidth > 0 {
sWidth := 0
if isWindows && inSelect {
sWidth = 1 // adjust for hightlighting on Windows
}
cLines = (cWidth + sWidth) / o.width
if (cWidth + sWidth) % o.width > 0 {
cLines = (cWidth + sWidth) / tWidth
if (cWidth + sWidth) % tWidth > 0 {
cLines++
}
}
Expand Down Expand Up @@ -403,25 +394,26 @@ func (o *opCompleter) pagerRefresh() (stayInMode bool) {
} else {
// after first page, redraw over --More--
buf.WriteString("\r")
}
}
buf.WriteString("\033[J") // clear anything below

same := o.op.buf.RuneSlice(-o.candidateOff)
sameWidth := runes.WidthAll(same)
tWidth, tHeight := o.w.GetWidthHeight()

colIdx := 0
lines := 1
for ; o.candidateChoise < len(o.candidate) ; o.candidateChoise++ {
c := o.candidate[o.candidateChoise]
cWidth := sameWidth + runes.WidthAll(c)
cLines := 1
if o.width > 0 {
cLines = cWidth / o.width
if cWidth % o.width > 0 {
if tWidth > 0 {
cLines = cWidth / tWidth
if cWidth % tWidth > 0 {
cLines++
}
}
if lines > 1 && lines + cLines > o.height {
if lines > 1 && lines + cLines > tHeight {
break // won't fit on page, stop early.
}
buf.WriteString(string(same))
Expand Down Expand Up @@ -460,7 +452,8 @@ func (o *opCompleter) pagerRefresh() (stayInMode bool) {
// we rewrite the prompt it does not over write the page content. The code to rewrite
// the prompt assumes the cursor is at the index line, so we add enough blank lines.
func (o *opCompleter) scrollOutOfPagerMode() {
lineCnt := o.op.buf.IdxLine(o.width)
tWidth, _ := o.w.GetWidthHeight()
lineCnt := o.op.buf.IdxLine(tWidth)
if lineCnt > 0 {
buf := bufio.NewWriter(o.w)
buf.Write(bytes.Repeat([]byte("\n"), lineCnt))
Expand Down
21 changes: 7 additions & 14 deletions operation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,24 +67,18 @@ func (w *wrapWriter) Write(b []byte) (int, error) {
}

func NewOperation(t *Terminal, cfg *Config) *Operation {
width, height := cfg.FuncGetSize()
op := &Operation{
t: t,
buf: NewRuneBuffer(t, cfg.Prompt, cfg, width, height),
buf: NewRuneBuffer(t, cfg.Prompt, cfg),
outchan: make(chan []rune),
errchan: make(chan error, 1),
}
op.w = op.buf.w
op.SetConfig(cfg)
op.opVim = newVimMode(op)
op.opCompleter = newOpCompleter(op.buf.w, op, width, height)
op.opCompleter = newOpCompleter(op.buf.w, op)
op.opPassword = newOpPassword(op)
op.cfg.FuncOnWidthChanged(func() {
newWidth, newHeight := cfg.FuncGetSize()
op.opCompleter.OnSizeChange(newWidth, newHeight)
op.opSearch.OnSizeChange(newWidth, newHeight)
op.buf.OnSizeChange(newWidth, newHeight)
})
op.cfg.FuncOnWidthChanged(t.OnSizeChange)
go op.ioloop()
return op
}
Expand Down Expand Up @@ -410,7 +404,7 @@ func (o *Operation) Runes() ([]rune, error) {
// maybe existing text on the same line that ideally we don't
// want to overwrite and cause prompt to jump left. Note that
// this is not perfect but works the majority of the time.
o.buf.getAndSetOffset(o.t)
o.buf.getAndSetOffset()
o.buf.Print() // print prompt & buffer contents
o.t.KickRead()

Expand Down Expand Up @@ -494,21 +488,20 @@ func (op *Operation) SetConfig(cfg *Config) (*Config, error) {
op.SetPrompt(cfg.Prompt)
op.SetMaskRune(cfg.MaskRune)
op.buf.SetConfig(cfg)
width, height := op.cfg.FuncGetSize()

if cfg.opHistory == nil {
op.SetHistoryPath(cfg.HistoryFile)
cfg.opHistory = op.history
cfg.opSearch = newOpSearch(op.buf.w, op.buf, op.history, cfg, width, height)
cfg.opSearch = newOpSearch(op.buf.w, op.buf, op.history, cfg)
}
op.history = cfg.opHistory

// SetHistoryPath will close opHistory which already exists
// so if we use it next time, we need to reopen it by `InitHistory()`
op.history.Init()

if op.cfg.AutoComplete != nil {
op.opCompleter = newOpCompleter(op.buf.w, op, width, height)
if op.cfg.AutoComplete != nil && op.opCompleter == nil {
op.opCompleter = newOpCompleter(op.buf.w, op)
}

op.opSearch = cfg.opSearch
Expand Down
50 changes: 18 additions & 32 deletions runebuf.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,11 @@ type RuneBuffer struct {
buf []rune
idx int
prompt []rune
w io.Writer
w *Terminal

interactive bool
cfg *Config

width int
height int

bck *runeBufferBck

offset string // is offset useful? scrolling means row varies
Expand All @@ -40,19 +37,6 @@ func (r *RuneBuffer) pushKill(text []rune) {
r.lastKill = append([]rune{}, text...)
}

func (r *RuneBuffer) OnWidthChange(newWidth int) {
r.Lock()
r.width = newWidth
r.Unlock()
}

func (r *RuneBuffer) OnSizeChange(newWidth, newHeight int) {
r.Lock()
r.width = newWidth
r.height = newHeight
r.Unlock()
}

func (r *RuneBuffer) Backup() {
r.Lock()
r.bck = &runeBufferBck{r.buf, r.idx}
Expand All @@ -69,13 +53,11 @@ func (r *RuneBuffer) Restore() {
})
}

func NewRuneBuffer(w io.Writer, prompt string, cfg *Config, width int, height int) *RuneBuffer {
func NewRuneBuffer(w *Terminal, prompt string, cfg *Config) *RuneBuffer {
rb := &RuneBuffer{
w: w,
interactive: cfg.useInteractive(),
cfg: cfg,
width: width,
height: height,
}
rb.SetPrompt(prompt)
return rb
Expand All @@ -102,9 +84,8 @@ func (r *RuneBuffer) CurrentWidth(x int) int {

func (r *RuneBuffer) PromptLen() int {
r.Lock()
width := r.promptLen()
r.Unlock()
return width
defer r.Unlock()
return r.promptLen()
}

func (r *RuneBuffer) promptLen() int {
Expand Down Expand Up @@ -448,12 +429,13 @@ func (r *RuneBuffer) isInLineEdge() bool {
}

func (r *RuneBuffer) getSplitByLine(rs []rune, nextWidth int) [][]rune {
tWidth, _ := r.w.GetWidthHeight()
if r.cfg.EnableMask {
w := runes.Width(r.cfg.MaskRune)
masked := []rune(strings.Repeat(string(r.cfg.MaskRune), len(rs)))
return SplitByLine(runes.ColorFilter(r.prompt), masked, r.ppos, r.width, w)
return SplitByLine(runes.ColorFilter(r.prompt), masked, r.ppos, tWidth, w)
} else {
return SplitByLine(runes.ColorFilter(r.prompt), rs, r.ppos, r.width, nextWidth)
return SplitByLine(runes.ColorFilter(r.prompt), rs, r.ppos, tWidth, nextWidth)
}
}

Expand All @@ -476,7 +458,8 @@ func (r *RuneBuffer) idxLine(width int) int {
}

func (r *RuneBuffer) CursorLineCount() int {
return r.LineCount() - r.IdxLine(r.width)
tWidth, _ := r.w.GetWidthHeight()
return r.LineCount() - r.IdxLine(tWidth)
}

func (r *RuneBuffer) Refresh(f func()) {
Expand Down Expand Up @@ -506,7 +489,7 @@ func (r *RuneBuffer) refresh(f func()) {
// will write the offset back to us via stdin and there may already be
// other data in the stdin buffer ahead of it.
// This function is called at the start of readline each time.
func (r *RuneBuffer) getAndSetOffset(t *Terminal) {
func (r *RuneBuffer) getAndSetOffset() {
if !r.interactive {
return
}
Expand All @@ -517,7 +500,7 @@ func (r *RuneBuffer) getAndSetOffset(t *Terminal) {
// at the beginning of the next line.
r.w.Write([]byte(" \b"))
}
t.GetOffset(r.SetOffset)
r.w.GetOffset(r.SetOffset)
}

func (r *RuneBuffer) SetOffset(offset string) {
Expand All @@ -528,8 +511,9 @@ func (r *RuneBuffer) SetOffset(offset string) {

func (r *RuneBuffer) setOffset(offset string) {
r.offset = offset
if _, c, ok := (&escapeKeyPair{attr:offset}).Get2(); ok && c > 0 && c < r.width {
r.ppos = c - 1 // c should be 1..width
tWidth, _ := r.w.GetWidthHeight()
if _, c, ok := (&escapeKeyPair{attr:offset}).Get2(); ok && c > 0 && c < tWidth {
r.ppos = c - 1 // c should be 1..tWidth
} else {
r.ppos = 0
}
Expand Down Expand Up @@ -703,7 +687,8 @@ func (r *RuneBuffer) SetPrompt(prompt string) {
func (r *RuneBuffer) cleanOutput(w io.Writer, idxLine int) {
buf := bufio.NewWriter(w)

if r.width == 0 {
tWidth, _ := r.w.GetWidthHeight()
if tWidth == 0 {
buf.WriteString(strings.Repeat("\r\b", len(r.buf)+r.promptLen()))
buf.Write([]byte("\033[J"))
} else {
Expand All @@ -724,7 +709,8 @@ func (r *RuneBuffer) Clean() {
}

func (r *RuneBuffer) clean() {
r.cleanWithIdxLine(r.idxLine(r.width))
tWidth, _ := r.w.GetWidthHeight()
r.cleanWithIdxLine(r.idxLine(tWidth))
}

func (r *RuneBuffer) cleanWithIdxLine(idxLine int) {
Expand Down
Loading

0 comments on commit f0ae515

Please sign in to comment.