[GH-ISSUE #145] data races/synchronization #113

Closed
opened 2026-03-04 01:02:03 +03:00 by kerem · 9 comments
Owner

Originally created by @3cb on GitHub (Jul 27, 2018).
Original GitHub issue: https://github.com/rivo/tview/issues/145

I'm having trouble synchronizing updates to Primitives and was hoping someone might be able to point me in the right direction.

My goal is to stream websocket data into multiple Table primitives which are displayed one at a time. I'm using a List to switch between the Tables by adding and removing items from a Flex. Main.go gives a general idea of what I'm trying to do(https://github.com/3cb/cq/blob/master/main.go)

I think the events triggered by the List primitive are conflicting with the updates done in the goroutine's select block and don't know the simplest solution.

Originally created by @3cb on GitHub (Jul 27, 2018). Original GitHub issue: https://github.com/rivo/tview/issues/145 I'm having trouble synchronizing updates to Primitives and was hoping someone might be able to point me in the right direction. My goal is to stream websocket data into multiple `Table` primitives which are displayed one at a time. I'm using a `List` to switch between the Tables by adding and removing items from a `Flex`. Main.go gives a general idea of what I'm trying to do([https://github.com/3cb/cq/blob/master/main.go](url)) I think the events triggered by the `List` primitive are conflicting with the updates done in the goroutine's select block and don't know the simplest solution.
kerem closed this issue 2026-03-04 01:02:03 +03:00
Author
Owner

@rivo commented on GitHub (Jul 28, 2018):

Without having run your code myself, I'm guessing that by pressing on a list item, you sometimes don't see the table change, correct? I believe this is because the activation of a list item causes app.Draw() to be called immediately afterwards but your handling of that event happens in a separate goroutine. Most of the time, your goroutine will finish after app.Draw() has already been called. (Worst case, it messes up the execution of app.Draw() midway.)

There are probably multiple ways to solve this. I would suggest that you make the selected callback of the AddItem() function wait for the goroutine to finish processing the event. Something like this:

done := make(chan struct{})
// ...
list.AddItem("Overview", "", '1', func() {
	view <- overviewTbl
	<- done
})
// ...
case tbl := <-view:
	// ...
	done <- struct{}{}

Let me know if this helps.

<!-- gh-comment-id:408630942 --> @rivo commented on GitHub (Jul 28, 2018): Without having run your code myself, I'm guessing that by pressing on a list item, you sometimes don't see the table change, correct? I believe this is because the activation of a list item causes `app.Draw()` to be called immediately afterwards but your handling of that event happens in a separate goroutine. Most of the time, your goroutine will finish after `app.Draw()` has already been called. (Worst case, it messes up the execution of `app.Draw()` midway.) There are probably multiple ways to solve this. I would suggest that you make the `selected` callback of the `AddItem()` function wait for the goroutine to finish processing the event. Something like this: ```go done := make(chan struct{}) // ... list.AddItem("Overview", "", '1', func() { view <- overviewTbl <- done }) // ... case tbl := <-view: // ... done <- struct{}{} ``` Let me know if this helps.
Author
Owner

@rivo commented on GitHub (Aug 4, 2018):

Will reopen if there is still an open issue here.

<!-- gh-comment-id:410437311 --> @rivo commented on GitHub (Aug 4, 2018): Will reopen if there is still an open issue here.
Author
Owner

@3cb commented on GitHub (Aug 4, 2018):

Good catch. Incoming websocket data was triggering app.Draw() so frequently that the problem was rarely noticeable. Unfortunately I'm still having a problem. Simply scrolling through List items without triggering the selected callback sets off the race detector: https://gist.github.com/3cb/89ecc2ba65a60280650611b41a537226

Aside from adding mutexes to the Primitives, I'm thinking my only option would be to use app.SetInputCapture() and instead of forwarding the key events I could handle them manually in the same goroutine as the other updates.

<!-- gh-comment-id:410453078 --> @3cb commented on GitHub (Aug 4, 2018): Good catch. Incoming websocket data was triggering `app.Draw()` so frequently that the problem was rarely noticeable. Unfortunately I'm still having a problem. Simply scrolling through `List` items without triggering the `selected` callback sets off the race detector: https://gist.github.com/3cb/89ecc2ba65a60280650611b41a537226 Aside from adding mutexes to the Primitives, I'm thinking my only option would be to use `app.SetInputCapture()` and instead of forwarding the key events I could handle them manually in the same goroutine as the other updates.
Author
Owner

@rivo commented on GitHub (Aug 7, 2018):

The call stack in your gist surprises me. The app.Draw() function uses a mutex (see here) so it should never execute concurrently.

I'm a bit puzzled as to how you're getting this race condition. Are you using the latest version of tview?

<!-- gh-comment-id:410992885 --> @rivo commented on GitHub (Aug 7, 2018): The call stack in your gist surprises me. The `app.Draw()` function uses a mutex (see [here](https://github.com/rivo/tview/blob/21f50f5bc400083b4eb23304887d9cd0fc00d075/application.go#L230)) so it should never execute concurrently. I'm a bit puzzled as to how you're getting this race condition. Are you using the latest version of `tview`?
Author
Owner

@3cb commented on GitHub (Aug 7, 2018):

I was a few commits behind. I think this commit eliminated part of the problem:
https://gist.github.com/3cb/9b847cebe16f363a232ce18b65b0c00d

The first four listed seem to be due to updating the Table primitive directly in a separate goroutine before app.Draw() is called:

// direclty update instances of Table
upd.UpdOverviewRow(overviewTbl)
upd.UpdRow(gdaxTbl)
app.Draw()

As I mentioned in the previous post the options I see are:

  1. Add mutexes to primitives
  2. Use app.SetInputCapture() to intercept key events and handle updates to List primitive in my own code

The last race listed is interesting because app.Run() seems to be conflicting with app.Draw():

==================
WARNING: DATA RACE
Read at 0x00c4201df880 by goroutine 178:
  github.com/rivo/tview.(*List).Draw()
      /home/marc/go/src/github.com/rivo/tview/list.go:225 +0x2e0
  github.com/rivo/tview.(*Flex).Draw()
      /home/marc/go/src/github.com/rivo/tview/flex.go:173 +0x728
  github.com/rivo/tview.(*Application).Draw()
      /home/marc/go/src/github.com/rivo/tview/application.go:259 +0x160
  main.main.func6()
      /home/marc/go/src/github.com/3cb/cq/main.go:72 +0x4c1

Previous write at 0x00c4201df880 by main goroutine:
  github.com/rivo/tview.(*List).InputHandler.func1()
      /home/marc/go/src/github.com/rivo/tview/list.go:287 +0xeb
  github.com/rivo/tview.(*Box).WrapInputHandler.func1()
      /home/marc/go/src/github.com/rivo/tview/box.go:152 +0xa6
  github.com/rivo/tview.(*Application).Run()
      /home/marc/go/src/github.com/rivo/tview/application.go:146 +0x467
  main.main()
      /home/marc/go/src/github.com/3cb/cq/main.go:89 +0x1322

Goroutine 178 (running) created at:
  main.main()
      /home/marc/go/src/github.com/3cb/cq/main.go:64 +0x12e5
==================

The two options above would also solve this I think but I could be missing something easier. What are your thoughts?

<!-- gh-comment-id:411173254 --> @3cb commented on GitHub (Aug 7, 2018): I was a few commits behind. I think [this commit](https://github.com/rivo/tview/commit/cc64ead1ed59123faf6d6f37ae2aa1a51e4af4fe) eliminated part of the problem: https://gist.github.com/3cb/9b847cebe16f363a232ce18b65b0c00d The first four listed seem to be due to updating the `Table` primitive directly in a separate goroutine before `app.Draw()` is called: ```go // direclty update instances of Table upd.UpdOverviewRow(overviewTbl) upd.UpdRow(gdaxTbl) app.Draw() ``` As I mentioned in the previous post the options I see are: 1. Add mutexes to primitives 2. Use `app.SetInputCapture()` to intercept key events and handle updates to `List` primitive in my own code The last race listed is interesting because `app.Run()` seems to be conflicting with `app.Draw()`: ``` ================== WARNING: DATA RACE Read at 0x00c4201df880 by goroutine 178: github.com/rivo/tview.(*List).Draw() /home/marc/go/src/github.com/rivo/tview/list.go:225 +0x2e0 github.com/rivo/tview.(*Flex).Draw() /home/marc/go/src/github.com/rivo/tview/flex.go:173 +0x728 github.com/rivo/tview.(*Application).Draw() /home/marc/go/src/github.com/rivo/tview/application.go:259 +0x160 main.main.func6() /home/marc/go/src/github.com/3cb/cq/main.go:72 +0x4c1 Previous write at 0x00c4201df880 by main goroutine: github.com/rivo/tview.(*List).InputHandler.func1() /home/marc/go/src/github.com/rivo/tview/list.go:287 +0xeb github.com/rivo/tview.(*Box).WrapInputHandler.func1() /home/marc/go/src/github.com/rivo/tview/box.go:152 +0xa6 github.com/rivo/tview.(*Application).Run() /home/marc/go/src/github.com/rivo/tview/application.go:146 +0x467 main.main() /home/marc/go/src/github.com/3cb/cq/main.go:89 +0x1322 Goroutine 178 (running) created at: main.main() /home/marc/go/src/github.com/3cb/cq/main.go:64 +0x12e5 ================== ``` The two options above would also solve this I think but I could be missing something easier. What are your thoughts?
Author
Owner

@3cb commented on GitHub (Sep 18, 2018):

@rivo I just got back to this. I went ahead and added mutexes to the primitives I was using (Table and List) as well as TableCell to correct the data races. Any interest in adding this to all primitives to make synchronization easier?

<!-- gh-comment-id:422503158 --> @3cb commented on GitHub (Sep 18, 2018): @rivo I just got back to this. I went ahead and added mutexes to the primitives I was using (`Table` and `List`) as well as `TableCell` to correct the data races. Any interest in adding this to all primitives to make synchronization easier?
Author
Owner

@rivo commented on GitHub (Sep 25, 2018):

I started adding mutexes to these classes myself (I think I'd go for a slightly different solution than in your PR) but then stopped halfway. It adds a lot of boilerplate code to almost all functions throughout the package and I'm not sure it's worth it. In the applications I wrote myself using tview, it was never needed.

At the moment I'm considering putting a disclaimer into the package description itself that using the functions of the package in multiple goroutines will require outside synchronization.

So I'm a bit torn now. Which is to say that I haven't made up my mind yet.

What would it take for you to synchronize access to these objects in your application? Do you know any popular Go packages I could look at, that ensure thread-safety for all getters and setters (and other functions) in all objects?

<!-- gh-comment-id:424438234 --> @rivo commented on GitHub (Sep 25, 2018): I started adding mutexes to these classes myself (I think I'd go for a slightly different solution than in your PR) but then stopped halfway. It adds a lot of boilerplate code to almost all functions throughout the package and I'm not sure it's worth it. In the applications I wrote myself using `tview`, it was never needed. At the moment I'm considering putting a disclaimer into the package description itself that using the functions of the package in multiple goroutines will require outside synchronization. So I'm a bit torn now. Which is to say that I haven't made up my mind yet. What would it take for you to synchronize access to these objects in your application? Do you know any popular Go packages I could look at, that ensure thread-safety for all getters and setters (and other functions) in all objects?
Author
Owner

@3cb commented on GitHub (Sep 25, 2018):

I actually haven't submitted a PR. I think you got mixed up with @joegrasse and his PR. Although we are both having problems syncing changes to Table it seems.

For my application, all I did was add mutexes and then only updated the methods I use with lock/unlock; I haven't bothered with unused methods so I understand your hesitation to add all that code.

An alternative that I was thinking about is to add a channel to Application typed as an Updater interface that would allow the user to inject updates to primitives into the event loop. What do you think about that?

<!-- gh-comment-id:424461103 --> @3cb commented on GitHub (Sep 25, 2018): I actually haven't submitted a PR. I think you got mixed up with @joegrasse and his PR. Although we are both having problems syncing changes to `Table` it seems. For my application, all I did was add mutexes and then only updated the methods I use with lock/unlock; I haven't bothered with unused methods so I understand your hesitation to add all that code. An alternative that I was thinking about is to add a channel to `Application` typed as an `Updater` interface that would allow the user to inject updates to primitives into the event loop. What do you think about that?
Author
Owner

@rivo commented on GitHub (Sep 26, 2018):

Oops, right. I didn't realize that. I'll post this in the other thread, then.

I'm not so familiar with the Updater interface. If you know a package that implements something like that, maybe you can point me to it?

<!-- gh-comment-id:424642033 --> @rivo commented on GitHub (Sep 26, 2018): Oops, right. I didn't realize that. I'll post this in the other thread, then. I'm not so familiar with the `Updater` interface. If you know a package that implements something like that, maybe you can point me to it?
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/tview#113
No description provided.