Skip to content

Commit 5e244a9

Browse files
authored
Fix "Jump to File" menu showing only half of the entries, in some rare cases (#6136)
When opening the `<ctrl+g>` files menu exactly at the moment where a background fetch finishes, it could sometimes only show some of its entries. The same could happen when pressing `>` to jump to the end of a long diff at that moment.
2 parents 4c2ce23 + b8b9c8a commit 5e244a9

4 files changed

Lines changed: 276 additions & 70 deletions

File tree

‎pkg/tasks/read_request_queue.go‎

Lines changed: 49 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,12 @@ import "sync"
1515
// The reader coming and going is the other half of what it's for. A request is
1616
// how a caller asks for content to be read and hears, through the request's Then,
1717
// that it has been; a request nobody answers leaves that caller waiting for good.
18-
// So asking whether a task is there and handing it the request are one step, and
19-
// so are taking the task away and handing back what it never answered. A request
20-
// made in between finds no task and goes back to its caller to answer.
18+
// A caller acts on the content the view shows once it is answered, so a request
19+
// is held for the task asked for last, from the moment it is asked for. When a
20+
// task is replaced, the requests it hasn't answered stay for the task replacing it.
21+
// Only once the task asked for last has given the view all of its content does the
22+
// queue stop holding requests, and hand back the ones it still holds to answer. A
23+
// request made after that finds no task and goes back to its caller to answer.
2124
//
2225
// enqueue appends under the mutex and rings the doorbell; the task selects on the
2326
// doorbell to wake, then takes requests until there are none left. The doorbell is
@@ -30,54 +33,82 @@ type readRequestQueue struct {
3033
requests []LinesToRead
3134
doorbell chan struct{}
3235

33-
// Whether a task is there to serve the requests. False before the first task
34-
// starts, and between one task ending and the next starting.
35-
serving bool
36+
// Whether requests are held for a task to serve: the one reading the content, or
37+
// one that has been asked for and is going to replace it. False before the first
38+
// task is asked for, and once the task asked for last has read all of its
39+
// content.
40+
holding bool
3641
}
3742

3843
func newReadRequestQueue() *readRequestQueue {
3944
return &readRequestQueue{doorbell: make(chan struct{}, 1)}
4045
}
4146

42-
// beginServing says that a task is now there to serve the queue, and returns the
43-
// doorbell that tells it when there is something to serve.
44-
func (self *readRequestQueue) beginServing() <-chan struct{} {
47+
// hold says that a task has been asked for, so that requests are held for it until
48+
// it serves them.
49+
func (self *readRequestQueue) hold() {
4550
self.mutex.Lock()
4651
defer self.mutex.Unlock()
4752

48-
self.serving = true
53+
self.holding = true
54+
}
55+
56+
// beginServing says that a task is now there to serve the queue, and returns the
57+
// doorbell that tells it when there is something to serve. The task's own first
58+
// request is served first, ahead of those it takes over from the task it replaces,
59+
// so that it shows its content as soon as it would have without them.
60+
func (self *readRequestQueue) beginServing(firstRequest LinesToRead) <-chan struct{} {
61+
self.mutex.Lock()
62+
self.holding = true
63+
self.requests = append([]LinesToRead{firstRequest}, self.requests...)
64+
self.mutex.Unlock()
65+
66+
self.ringDoorbell()
4967
return self.doorbell
5068
}
5169

52-
// stopServing takes the task away and hands back the requests it never answered,
53-
// for the caller to answer in its place.
70+
// stopServing stops holding requests and hands back the ones it still holds, for
71+
// the caller to answer in place of a task.
5472
func (self *readRequestQueue) stopServing() []LinesToRead {
5573
self.mutex.Lock()
5674
defer self.mutex.Unlock()
5775

58-
self.serving = false
76+
self.holding = false
5977
unanswered := self.requests
6078
self.requests = nil
6179
return unanswered
6280
}
6381

64-
// enqueue gives a request to the task serving the queue, and reports whether
65-
// there was one to give it to. When there wasn't, the request is the caller's to
66-
// answer.
82+
// enqueue gives a request to the task that is going to serve it, and reports
83+
// whether there is one. When there isn't, the request is the caller's to answer.
6784
func (self *readRequestQueue) enqueue(request LinesToRead) bool {
6885
self.mutex.Lock()
69-
if !self.serving {
86+
if !self.holding {
7087
self.mutex.Unlock()
7188
return false
7289
}
7390
self.requests = append(self.requests, request)
7491
self.mutex.Unlock()
7592

93+
self.ringDoorbell()
94+
return true
95+
}
96+
97+
// requeue puts back a request that a task was serving when it was stopped, ahead of
98+
// the ones made after it, for the task replacing it to serve.
99+
func (self *readRequestQueue) requeue(request LinesToRead) {
100+
self.mutex.Lock()
101+
self.requests = append([]LinesToRead{request}, self.requests...)
102+
self.mutex.Unlock()
103+
104+
self.ringDoorbell()
105+
}
106+
107+
func (self *readRequestQueue) ringDoorbell() {
76108
select {
77109
case self.doorbell <- struct{}{}:
78110
default:
79111
}
80-
return true
81112
}
82113

83114
// dequeue takes the oldest request, reporting false when there are none.

‎pkg/tasks/read_request_queue_test.go‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,41 +9,60 @@ import (
99
func TestReadRequestQueueHandsBackWhatNoTaskWillServe(t *testing.T) {
1010
queue := newReadRequestQueue()
1111

12-
// Nothing has begun serving, so the request comes straight back to its caller.
12+
// No task has been asked for, so the request comes straight back to its caller.
1313
assert.False(t, queue.enqueue(LinesToRead{Total: 1}))
1414

15-
queue.beginServing()
16-
assert.True(t, queue.enqueue(LinesToRead{Total: 1}))
15+
// Once one has, requests are held for it, and the task serves its own first
16+
// request ahead of them.
17+
queue.hold()
1718
assert.True(t, queue.enqueue(LinesToRead{Total: 2}))
19+
queue.beginServing(LinesToRead{Total: 1})
20+
assert.True(t, queue.enqueue(LinesToRead{Total: 3}))
1821

1922
request, ok := queue.dequeue()
2023
assert.True(t, ok)
2124
assert.Equal(t, 1, request.Total)
2225

23-
// What the task never got to comes back when it stops, and nothing is taken
24-
// from a caller after that.
26+
// A request that a task was serving when it was stopped goes back ahead of the
27+
// ones made after it.
28+
queue.requeue(request)
29+
request, _ = queue.dequeue()
30+
assert.Equal(t, 1, request.Total)
31+
request, _ = queue.dequeue()
32+
assert.Equal(t, 2, request.Total)
33+
34+
// What no task got to comes back when the queue stops holding requests, and
35+
// nothing is taken from a caller after that.
2536
unanswered := queue.stopServing()
2637
assert.Len(t, unanswered, 1)
27-
assert.Equal(t, 2, unanswered[0].Total)
38+
assert.Equal(t, 3, unanswered[0].Total)
2839

29-
assert.False(t, queue.enqueue(LinesToRead{Total: 3}))
40+
assert.False(t, queue.enqueue(LinesToRead{Total: 4}))
3041
_, ok = queue.dequeue()
3142
assert.False(t, ok)
3243
}
3344

3445
func TestReadRequestQueueRingsTheDoorbell(t *testing.T) {
3546
queue := newReadRequestQueue()
36-
doorbell := queue.beginServing()
47+
doorbell := queue.beginServing(LinesToRead{Total: 1})
48+
49+
// The task's first request is there to be served right away.
50+
select {
51+
case <-doorbell:
52+
default:
53+
assert.Fail(t, "the doorbell didn't ring for the first request")
54+
}
55+
_, _ = queue.dequeue()
3756

3857
select {
3958
case <-doorbell:
40-
assert.Fail(t, "the doorbell rang before anything was queued")
59+
assert.Fail(t, "the doorbell rang with nothing queued")
4160
default:
4261
}
4362

4463
// A burst leaves one token, which stands for everything queued.
45-
queue.enqueue(LinesToRead{Total: 1})
4664
queue.enqueue(LinesToRead{Total: 2})
65+
queue.enqueue(LinesToRead{Total: 3})
4766

4867
select {
4968
case <-doorbell:

‎pkg/tasks/tasks.go‎

Lines changed: 50 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -435,11 +435,30 @@ func (self *ViewBufferManager) readHoldingATask(totalLines int, then func()) {
435435
}
436436
}
437437

438-
// stopServingReadRequests takes the task away from the read-request queue and
439-
// answers whatever it never got to, so that nobody is left waiting for a callback
440-
// that isn't coming.
438+
// stopServingReadRequests stops holding read requests for a task and answers the
439+
// ones still waiting, so that nobody is left waiting for a callback that isn't
440+
// coming.
441441
func (self *ViewBufferManager) stopServingReadRequests() {
442-
for _, request := range self.readRequests.stopServing() {
442+
answerReadRequests(self.readRequests.stopServing())
443+
}
444+
445+
// answerReadRequestsIfLatest answers the read requests still waiting once the given
446+
// task has given the view all of its content, unless another task has been asked for
447+
// since. The requests are waiting for that task's content then, so they are left for
448+
// it to serve.
449+
func (self *ViewBufferManager) answerReadRequestsIfLatest(taskID int) {
450+
self.taskIDMutex.Lock()
451+
var unanswered []LinesToRead
452+
if taskID == self.newTaskID {
453+
unanswered = self.readRequests.stopServing()
454+
}
455+
self.taskIDMutex.Unlock()
456+
457+
answerReadRequests(unanswered)
458+
}
459+
460+
func answerReadRequests(requests []LinesToRead) {
461+
for _, request := range requests {
443462
if request.Then != nil {
444463
request.Then()
445464
}
@@ -545,9 +564,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
545564

546565
loadingMutex := deadlock.Mutex{}
547566

548-
// Begin serving before any goroutine starts, so that the first request below
549-
// can't arrive before there is a task to take it.
550-
readRequests := self.readRequests.beginServing()
567+
readRequests := self.readRequests.beginServing(initialRequest)
551568

552569
scanner := bufio.NewScanner(r)
553570
scanner.Split(utils.ScanLinesAndTruncateWhenLongerThanBuffer(bufio.MaxScanTokenSize))
@@ -705,6 +722,15 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
705722
linesToRead.Then()
706723
}
707724
}
725+
// A task that is stopped is being replaced by one that renders the
726+
// content the caller is going to act on, so the request is left for
727+
// that one to serve. A request without a Then has nobody waiting on
728+
// it, and the task replacing this one reads what it needs by itself.
729+
handOver := func() {
730+
if linesToRead.Then != nil {
731+
self.readRequests.requeue(linesToRead)
732+
}
733+
}
708734
// A restore that hasn't painted yet keeps us reading past the lines
709735
// asked for, all the way to the end of the input if need be. What it
710736
// is looking for may be anywhere in the new content, and a rendering
@@ -713,14 +739,14 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
713739
// and the view somewhere the user didn't put it.
714740
for linesToRead.Total == -1 || linesRead < linesToRead.Total || (restore != nil && !painted) {
715741
if stopped() {
716-
callThen()
742+
handOver()
717743
break outer
718744
}
719745
var ok bool
720746
var line []byte
721747
select {
722748
case <-opts.Stop:
723-
callThen()
749+
handOver()
724750
break outer
725751
case line, ok = <-lineChan:
726752
// process line below
@@ -752,7 +778,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
752778
// case above.
753779
select {
754780
case <-opts.Stop:
755-
callThen()
781+
handOver()
756782
break outer
757783
default:
758784
}
@@ -805,10 +831,10 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
805831
}
806832

807833
// Whoever made a request the loop never got to is waiting to hear that the
808-
// content it asked for has been read, and there is nothing here to read it
809-
// any more: at end of input it has all been read already, and a task that
810-
// was stopped is handing the view over to the one replacing it.
811-
self.stopServingReadRequests()
834+
// content it asked for has been read. At end of input it has all been read
835+
// already. A task that was stopped leaves the requests to the one replacing
836+
// it.
837+
self.answerReadRequestsIfLatest(opts.taskID)
812838

813839
refreshViewIfStale()
814840

@@ -832,8 +858,6 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
832858
close(lineWrittenChan)
833859
})
834860

835-
self.readRequests.enqueue(initialRequest)
836-
837861
<-done
838862

839863
return nil
@@ -842,6 +866,10 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
842866

843867
// Close closes the task manager, killing whatever task may currently be running
844868
func (self *ViewBufferManager) Close() {
869+
// No task is going to read anything after this, so nobody may be left waiting
870+
// for content to be read.
871+
defer self.stopServingReadRequests()
872+
845873
// stopCurrentTask is written by NewTask's goroutine under waitingMutex (and
846874
// so is the sync.Once it closes over), so read it under the lock and call
847875
// the captured value; a task starting on shutdown must not race us here.
@@ -900,6 +928,8 @@ func (self *ViewBufferManager) ReserveTask() TaskReservation {
900928
defer self.taskIDMutex.Unlock()
901929

902930
self.newTaskID++
931+
// Whoever asks for content to be read from now on is asking for this task's.
932+
self.readRequests.hold()
903933
return TaskReservation{taskID: self.newTaskID}
904934
}
905935

@@ -986,9 +1016,6 @@ func (self *ViewBufferManager) NewReservedTask(reservation TaskReservation, f fu
9861016
self.stopCurrentTask()
9871017
}
9881018

989-
// Nothing serves read requests between one task and the next.
990-
self.stopServingReadRequests()
991-
9921019
stop := make(chan struct{})
9931020
notifyStopped := make(chan struct{})
9941021

@@ -1006,6 +1033,10 @@ func (self *ViewBufferManager) NewReservedTask(reservation TaskReservation, f fu
10061033
self.Log.Error(err) // might need an onError callback
10071034
}
10081035

1036+
// A task that reads no command's output, such as one showing a message, has
1037+
// given the view all of its content once it returns.
1038+
self.answerReadRequestsIfLatest(taskID)
1039+
10091040
close(notifyStopped)
10101041
})
10111042

0 commit comments

Comments
 (0)