[GH-ISSUE #148] panic: tview.(*Table).Draw.func1 #116

New issue

Closed

opened 2026-03-04 01:02:04 +03:00 by kerem · 28 comments

kerem commented 2026-03-04 01:02:04 +03:00

Owner

Copy link

Originally created by @joegrasse on GitHub (Jul 31, 2018).
Original GitHub issue: https://github.com/rivo/tview/issues/148

I am getting a panic in tview. It looks like it might be in table.go line 469, but it is really hard to troubleshoot because of the way the panic is being printed.

(Click image for bigger view)

Originally created by @joegrasse on GitHub (Jul 31, 2018). Original GitHub issue: https://github.com/rivo/tview/issues/148 I am getting a panic in tview. It looks like it might be in table.go line [469](https://github.com/rivo/tview/blob/6614b16d9037759065eac63d8fb3d8408e63e222/table.go#L469), but it is really hard to troubleshoot because of the way the panic is being printed.  (Click image for bigger view)

kerem closed this issue 2026-03-04 01:02:05 +03:00

kerem commented 2026-03-04 01:02:05 +03:00

Author

Owner

Copy link

@ardnew commented on GitHub (Jul 31, 2018):

This will be basically impossible to debug without more information. You'll have to post some relevant source code for any chance of help.

If you're having trouble reading the panic message, you can trap the panic using defer() and recover() (see: Defer, Panic, and Recover, and Handling panics), and then from there print the panic message and any other debug info (e.g. the call stack) safely to a log file or somewhere.

<!-- gh-comment-id:409368671 --> @ardnew commented on GitHub (Jul 31, 2018): This will be basically impossible to debug without more information. You'll have to post some relevant source code for any chance of help. If you're having trouble reading the panic message, you can trap the panic using `defer()` and `recover()` (see: [Defer, Panic, and Recover](https://blog.golang.org/defer-panic-and-recover), and [Handling panics](https://golang.org/ref/spec#Handling_panics)), and then from there print the panic message and any other debug info (e.g. [the call stack](https://golang.org/pkg/runtime/debug/#Stack)) safely to a log file or somewhere.

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Aug 1, 2018):

As stated above the source code is tview table.go I believe. I have added a link above.

I will see if I can capture more information via defer() and recover(). Thanks for the recommendation.

<!-- gh-comment-id:409541226 --> @joegrasse commented on GitHub (Aug 1, 2018): As stated above the source code is tview table.go I believe. I have added a link above. I will see if I can capture more information via ```defer()``` and ```recover()```. Thanks for the recommendation.

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

@ardnew commented on GitHub (Aug 1, 2018):

table.go:469 is only the point at which the actual fault occurs. If you look at the source, you'll see that line is the body of a closure -- for a function ref getCell that is called from 4 different locations. So with only that source reference, we have no way of knowing where the fault originated. You need to obtain the call stack to determine how it arrived at that fault location.

But even with the call stack in hand, it's more likely the real problem is in the table construction (your code, not the library code). So if you can share any code showing how you're constructing the table, that would be very helpful.

It would also be helpful if you could of course describe how the fault appears. Does it happen immediately when the table is first drawn? After you select a cell? Remove a cell? Does it happen seemingly randomly?

<!-- gh-comment-id:409613286 --> @ardnew commented on GitHub (Aug 1, 2018): table.go:469 is only the point at which the actual fault occurs. If you look at the source, you'll see that line is the body of a closure -- for a function ref `getCell` that is called from 4 different locations. So with only that source reference, we have no way of knowing where the fault originated. You need to obtain the call stack to determine how it arrived at that fault location. But even with the call stack in hand, it's more likely the real problem is in the table construction (your code, not the library code). So if you can share any code showing how you're constructing the table, that would be very helpful. It would also be helpful if you could of course describe how the fault appears. Does it happen immediately when the table is first drawn? After you select a cell? Remove a cell? Does it happen seemingly randomly?

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Aug 1, 2018):

I have added the defer() and recover(). This issue happens at random. Hopefully the next time it happens I will have more information to provide.

<!-- gh-comment-id:409637739 --> @joegrasse commented on GitHub (Aug 1, 2018): I have added the ```defer()``` and ```recover()```. This issue happens at random. Hopefully the next time it happens I will have more information to provide.

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

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

Are you calling Table.Draw() directly from some goroutine? The Application.Draw() function recovers any panics and prints it after closing the tcell session. Three things make me think you're calling Table's Draw() function by yourself:

• The messed up way the panic is printed looks like the tcell session has not been properly terminated.
• The top of your call stack is not the part of Application where panics are recovered.

I could be wrong but maybe you can clarify what you're doing.

<!-- gh-comment-id:410474160 --> @rivo commented on GitHub (Aug 4, 2018): Are you calling `Table.Draw()` directly from some goroutine? The `Application.Draw()` function recovers any panics and prints it after closing the `tcell` session. Three things make me think you're calling `Table`'s `Draw()` function by yourself: - The messed up way the panic is printed looks like the `tcell` session has not been properly terminated. - The top of your call stack is not the part of `Application` where panics are recovered. I could be wrong but maybe you can clarify what you're doing.

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Aug 6, 2018):

No I am not calling Table.Draw(). The only draw method I am calling is the application.Draw() function.

<!-- gh-comment-id:410694305 --> @joegrasse commented on GitHub (Aug 6, 2018): No I am not calling ```Table.Draw()```. The only draw method I am calling is the ```application.Draw()``` function.

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

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

Ok, well, then let's see what your next recover() comes up with.

<!-- gh-comment-id:410956908 --> @rivo commented on GitHub (Aug 7, 2018): Ok, well, then let's see what your next `recover()` comes up with.

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Aug 14, 2018):

@rivo I have not had the problem since adding defer() and recover(), and normally I would have had it several times by now. Digging into why that change might have helped, I noticed I might have picked up this change in the recompile. Does it make sense that this might have fixed my issue?

<!-- gh-comment-id:412949994 --> @joegrasse commented on GitHub (Aug 14, 2018): @rivo I have not had the problem since adding ```defer()``` and ```recover()```, and normally I would have had it several times by now. Digging into why that change might have helped, I noticed I might have picked up [this](https://github.com/rivo/tview/commit/cc64ead1ed59123faf6d6f37ae2aa1a51e4af4fe) change in the recompile. Does it make sense that this might have fixed my issue?

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

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

There's a good chance that this fixed it. I thought you had already included this commit.

I'm closing this issue now. If you find that you still have problems, I'll reopen it.

<!-- gh-comment-id:413276565 --> @rivo commented on GitHub (Aug 15, 2018): There's a good chance that this fixed it. I thought you had already included this commit. I'm closing this issue now. If you find that you still have problems, I'll reopen it.

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Sep 4, 2018):

@rivo This issue ended up happening again. Just not quite as often. I had to edit table.go to capture the panic.

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x6a0440]

goroutine 6 [running]:
github.com/rivo/tview.(*Table).Draw.func1(0x8232e0, 0xc042126d80)
D:/gowork/src/github.com/rivo/tview/table.go:459 +0x86
panic(0x77b820, 0x9ccda0)
C:/Go/src/runtime/panic.go:502 +0x237
github.com/rivo/tview.(*Table).Draw.func2(...)
D:/gowork/src/github.com/rivo/tview/table.go:478
github.com/rivo/tview.(*Table).Draw(0xc042059360, 0x8232e0, 0xc042126d80)
D:/gowork/src/github.com/rivo/tview/table.go:792 +0x1eb0
github.com/rivo/tview.(*Pages).Draw(0xc0420f9d40, 0x8232e0, 0xc042126d80)
D:/gowork/src/github.com/rivo/tview/pages.go:246 +0xfd
github.com/rivo/tview.(*Grid).Draw(0xc042122090, 0x8232e0, 0xc042126d80)
D:/gowork/src/github.com/rivo/tview/grid.go:631 +0x150b
github.com/rivo/tview.(*Pages).Draw(0xc0420f9d10, 0x8232e0, 0xc042126d80)
D:/gowork/src/github.com/rivo/tview/pages.go:246 +0xfd
github.com/rivo/tview.(*Application).Draw(0xc042108af0, 0x0)
D:/gowork/src/github.com/rivo/tview/application.go:259 +0xda
github.com/joegrasse/goburrow/goburrow/ui.NewApp.func2(0xc0421092d0)
D:/gowork/src/github.com/joegrasse/goburrow/goburrow/ui/app.go:184 +0x4f
created by github.com/joegrasse/goburrow/goburrow/ui.NewApp
D:/gowork/src/github.com/joegrasse/goburrow/goburrow/ui/app.go:181 +0xdc0

<!-- gh-comment-id:418504593 --> @joegrasse commented on GitHub (Sep 4, 2018): @rivo This issue ended up happening again. Just not quite as often. I had to edit table.go to capture the panic. panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal 0xc0000005 code=0x0 addr=0x0 pc=0x6a0440] goroutine 6 [running]: github.com/rivo/tview.(*Table).Draw.func1(0x8232e0, 0xc042126d80) D:/gowork/src/github.com/rivo/tview/table.go:459 +0x86 panic(0x77b820, 0x9ccda0) C:/Go/src/runtime/panic.go:502 +0x237 github.com/rivo/tview.(*Table).Draw.func2(...) D:/gowork/src/github.com/rivo/tview/table.go:478 github.com/rivo/tview.(*Table).Draw(0xc042059360, 0x8232e0, 0xc042126d80) D:/gowork/src/github.com/rivo/tview/table.go:792 +0x1eb0 github.com/rivo/tview.(*Pages).Draw(0xc0420f9d40, 0x8232e0, 0xc042126d80) D:/gowork/src/github.com/rivo/tview/pages.go:246 +0xfd github.com/rivo/tview.(*Grid).Draw(0xc042122090, 0x8232e0, 0xc042126d80) D:/gowork/src/github.com/rivo/tview/grid.go:631 +0x150b github.com/rivo/tview.(*Pages).Draw(0xc0420f9d10, 0x8232e0, 0xc042126d80) D:/gowork/src/github.com/rivo/tview/pages.go:246 +0xfd github.com/rivo/tview.(*Application).Draw(0xc042108af0, 0x0) D:/gowork/src/github.com/rivo/tview/application.go:259 +0xda github.com/joegrasse/goburrow/goburrow/ui.NewApp.func2(0xc0421092d0) D:/gowork/src/github.com/joegrasse/goburrow/goburrow/ui/app.go:184 +0x4f created by github.com/joegrasse/goburrow/goburrow/ui.NewApp D:/gowork/src/github.com/joegrasse/goburrow/goburrow/ui/app.go:181 +0xdc0

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

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

Could you provide me with a commit hash of the version you're using? I've made changes to table.go and the line numbers don't line up anymore. Thanks!

<!-- gh-comment-id:418698270 --> @rivo commented on GitHub (Sep 5, 2018): Could you provide me with a commit hash of the version you're using? I've made changes to `table.go` and the line numbers don't line up anymore. Thanks!

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Sep 5, 2018):

the line numbers don't match because i had to do the following to capture the panic.

diff --git a/table.go b/table.go
index 38f5006..7690aba 100644
--- a/table.go
+++ b/table.go
@@ -451,6 +451,15 @@ func (t *Table) ScrollToEnd() *Table {

 // Draw draws this primitive onto the screen.
 func (t *Table) Draw(screen tcell.Screen) {
+       defer func() {
+               if r := recover(); r != nil {
+                       if screen != nil {
+                               screen.Fini()
+                       }
+                       panic(r)
+               }
+       }()
+
        t.Box.Draw(screen)

        // What's our available screen space?

<!-- gh-comment-id:418706984 --> @joegrasse commented on GitHub (Sep 5, 2018): the line numbers don't match because i had to do the following to capture the panic. ```diff diff --git a/table.go b/table.go index 38f5006..7690aba 100644 --- a/table.go +++ b/table.go @@ -451,6 +451,15 @@ func (t *Table) ScrollToEnd() *Table { // Draw draws this primitive onto the screen. func (t *Table) Draw(screen tcell.Screen) { + defer func() { + if r := recover(); r != nil { + if screen != nil { + screen.Fini() + } + panic(r) + } + }() + t.Box.Draw(screen) // What's our available screen space? ```

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Sep 5, 2018):

Here is the commit hash I am using.

commit 21f50f5bc4

<!-- gh-comment-id:418709018 --> @joegrasse commented on GitHub (Sep 5, 2018): Here is the commit hash I am using. commit 21f50f5bc400083b4eb23304887d9cd0fc00d075

kerem commented 2026-03-04 01:02:06 +03:00

Author

Owner

Copy link

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

Thanks. To be honest, I cannot explain this. The message is invalid memory address or nil pointer dereference and it happens here:

return t.cells[row][column]

The problem doesn't appear to be the slice indices as (A) I check for them and (B) the error message would be different. So t seems to be nil. But how can that be? It's provided to the function by Go and it's never modified:

func (t *Table) Draw(screen tcell.Screen)

What I also find somewhat odd is that Pages calls Grid which calls Pages again. Do you have a Pages object in a Grid object which is again in a Pages object? (It's possible but unusual, I guess.)

You mentioned that you're not calling Draw() from any other goroutine.

I don't want to rule out that this is actually a compiler bug. But then, I've never come across one myself in decades of programming so the likelihood of that is low.

I can offer to take a look at your code. But it doesn't appear to be public so I'm not sure how we're going to do that.

<!-- gh-comment-id:418761743 --> @rivo commented on GitHub (Sep 5, 2018): Thanks. To be honest, I cannot explain this. The message is `invalid memory address or nil pointer dereference` and it happens here: ```go return t.cells[row][column] ``` The problem doesn't appear to be the slice indices as (A) I check for them and (B) the error message would be different. So `t` seems to be `nil`. But how can that be? It's provided to the function by Go and it's never modified: ```go func (t *Table) Draw(screen tcell.Screen) ``` What I also find somewhat odd is that `Pages` calls `Grid` which calls `Pages` again. Do you have a `Pages` object in a `Grid` object which is again in a `Pages` object? (It's possible but unusual, I guess.) You mentioned that you're not calling `Draw()` from any other goroutine. I don't want to rule out that this is actually a compiler bug. But then, I've never come across one myself in decades of programming so the likelihood of that is low. I can offer to take a look at your code. But it doesn't appear to be public so I'm not sure how we're going to do that.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Sep 5, 2018):

I am not calling Table.Draw, but I am calling Application.Draw() from multiple go routines. This is actually how I got it to happen more often, by calling Application.Draw() more often from those go routines.

Do you have a Pages object in a Grid object which is again in a Pages object?

Yes

I can offer to take a look at your code. But it doesn't appear to be public so I'm not sure how we're going to do that.

I do plan on publishing my code to github. I might just have to do that sooner than I wanted.

Let me see if I can help trace this down first, rather than rushing the publishing.

<!-- gh-comment-id:418784262 --> @joegrasse commented on GitHub (Sep 5, 2018): I am not calling ```Table.Draw```, but I am calling ```Application.Draw()``` from multiple go routines. This is actually how I got it to happen more often, by calling ```Application.Draw()``` more often from those go routines. >Do you have a Pages object in a Grid object which is again in a Pages object? Yes >I can offer to take a look at your code. But it doesn't appear to be public so I'm not sure how we're going to do that. I do plan on publishing my code to github. I might just have to do that sooner than I wanted. Let me see if I can help trace this down first, rather than rushing the publishing.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

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

This is actually how I got it to happen more often, by calling Application.Draw() more often from those go routines.

Application.Draw() holds a lock on the application object during its execution (see here) so I don't see how this is a problem.

I have a feeling there's something in plain sight which I'm simply not seeing...

<!-- gh-comment-id:418849234 --> @rivo commented on GitHub (Sep 5, 2018): > This is actually how I got it to happen more often, by calling Application.Draw() more often from those go routines. `Application.Draw()` holds a lock on the application object during its execution (see [here](https://github.com/rivo/tview/blob/21f50f5bc400083b4eb23304887d9cd0fc00d075/application.go#L230)) so I don't see how this is a problem. I have a feeling there's something in plain sight which I'm simply not seeing...

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Sep 6, 2018):

It looks like Table.cells is not guarded against concurrent writing/reading. There are several functions that can modify Table.cells (for example Table.Clear(), Table.SetCell(...)), while other functions are accessing it.

<!-- gh-comment-id:419094943 --> @joegrasse commented on GitHub (Sep 6, 2018): It looks like ```Table.cells``` is not guarded against concurrent writing/reading. There are several functions that can modify ```Table.cells``` (for example ```Table.Clear()```, ```Table.SetCell(...)```), while other functions are accessing it.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

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

Yes, that is correct. It was a deliberate decision not to wrap access to all member variables with mutex locks. (Although I'm certainly open to discussing this.) Some of the classes like TableCell give you direct access to their contents so it wouldn't even be possible without hiding them.

This is usually not a problem if changes to these objects are made in response to events generated by tview, e.g. keyboard events. In that case, your application essentially remains single-threaded.

But if changes are made in separate goroutines, you need to start syncing them. There are some functions which are thread-safe, like Application.Draw() or the Writer interface of TextView, for example. But the rest is basically up to you.

I was thinking about documenting this fact more and maybe providing examples on how to resolve common issues. Haven't gotten around to it yet.

Regarding your issue, however, the error message that you see still seems strange to me. But maybe race conditions can have unpredictable effects. I'm not familiar enough with Go's internals to be able to tell.

<!-- gh-comment-id:419410915 --> @rivo commented on GitHub (Sep 7, 2018): Yes, that is correct. It was a deliberate decision not to wrap access to all member variables with mutex locks. (Although I'm certainly open to discussing this.) Some of the classes like `TableCell` give you direct access to their contents so it wouldn't even be possible without hiding them. This is usually not a problem if changes to these objects are made in response to events generated by `tview`, e.g. keyboard events. In that case, your application essentially remains single-threaded. But if changes are made in separate goroutines, you need to start syncing them. There are some functions which are thread-safe, like `Application.Draw()` or the `Writer` interface of `TextView`, for example. But the rest is basically up to you. I was thinking about documenting this fact more and maybe providing examples on how to resolve common issues. Haven't gotten around to it yet. Regarding your issue, however, the error message that you see still seems strange to me. But maybe race conditions can have unpredictable effects. I'm not familiar enough with Go's internals to be able to tell.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Sep 7, 2018):

So, to give you a little more background about my application. I have a table that represents "things" running in the background. These "things" can change state. So, I have a goroutine that refreshes the table periodically to reflect the changing states. The end user can also modify the "things" in the table through the GUI by adding, remove, or modifing them.

So, while I am only calling Application.Draw() and no other Draw methods, I am inadvertently causing a race by calling Application.Draw() (which under the covers calls Table.Draw()) in one goroutine and the end user (or another goroutine) modifies the table contents in another. One goroutine could clear Table.cells (Table.Clear()), while another goroutine (Application.Draw() -> Table.Draw()) is trying to use Table.cells to draw it.

Back to the issue at hand, I think this would be really hard for a user of your library to grasp this intricacy, and understand when and where they need to sync. I believe you wouldn't have to wrap all member variables. you probably only need to wrap Table.cells and maybe Table.*Box in read and write mutexes. if you did this, this would be a major win for end users of your library. They wouldn't have to really understand the inner workings of a table to use it concurrently.

<!-- gh-comment-id:419433096 --> @joegrasse commented on GitHub (Sep 7, 2018): So, to give you a little more background about my application. I have a table that represents "things" running in the background. These "things" can change state. So, I have a goroutine that refreshes the table periodically to reflect the changing states. The end user can also modify the "things" in the table through the GUI by adding, remove, or modifing them. So, while I am only calling ```Application.Draw()``` and no other Draw methods, I am inadvertently causing a race by calling ```Application.Draw()``` (which under the covers calls ```Table.Draw()```) in one goroutine and the end user (or another goroutine) modifies the table contents in another. One goroutine could clear ```Table.cells``` (```Table.Clear()```), while another goroutine (```Application.Draw()``` -> ```Table.Draw()```) is trying to use ```Table.cells``` to draw it. Back to the issue at hand, I think this would be really hard for a user of your library to grasp this intricacy, and understand when and where they need to sync. I believe you wouldn't have to wrap all member variables. you probably only need to wrap ```Table.cells``` and maybe ```Table.*Box``` in read and write mutexes. if you did this, this would be a major win for end users of your library. They wouldn't have to really understand the inner workings of a table to use it concurrently.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Sep 7, 2018):

I have submitted a pull request for this issue.

<!-- gh-comment-id:419533812 --> @joegrasse commented on GitHub (Sep 7, 2018): I have submitted a pull request for this issue.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

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

I wrote a reply to your thread by posted it in the wrong issue. Please see my response there: https://github.com/rivo/tview/issues/145#issuecomment-424438234.

I don't think it's enough to just sync the functions of Table. Users could modify TableCell as well and it will lead to race conditions (same with ListItem and TreeNode).

@3cb suggests an Updater interface which I'm not sure yet what it would look like. But it could be explored to solve issues like yours. Maybe something like this:

func (a *Application) QueueUpdate(updater func()) Application

Then, whenever your application updates in a separate goroutine, instead of calling functions on Table directly, you do this:

app.QueueUpdate(func() {
    table.Clear()
})

It will then be called right before everything is redrawn (during the call to app.Draw()), in a thread-safe fashion.

Any thoughts?

<!-- gh-comment-id:424646151 --> @rivo commented on GitHub (Sep 26, 2018): I wrote a reply to your thread by posted it in the wrong issue. Please see my response there: https://github.com/rivo/tview/issues/145#issuecomment-424438234. I don't think it's enough to just sync the functions of `Table`. Users could modify `TableCell` as well and it will lead to race conditions (same with `ListItem` and `TreeNode`). @3cb suggests an `Updater` interface which I'm not sure yet what it would look like. But it could be explored to solve issues like yours. Maybe something like this: ```go func (a *Application) QueueUpdate(updater func()) Application ``` Then, whenever your application updates in a separate goroutine, instead of calling functions on `Table` directly, you do this: ```go app.QueueUpdate(func() { table.Clear() }) ``` It will then be called right before everything is redrawn (during the call to `app.Draw()`), in a thread-safe fashion. Any thoughts?

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

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

While I understand why you are hesitating on how to implement this, I would say that I am not quite a fan of the Updater interface as presented above. This seems like it causes more of a burden on the users of your library to understand the intricacy of how to use your library. This would cause questions like, "When do I need to use the Updater interface?", "What functions do I need to use with the Updater interface?", etc. Which in the end I think, would lead to more issues and hand holding.

For ease of use, I think the syncing should be done behind the scenes. Now how you do it behind the scenes (Updater, Sync), doesn't really matter to me.

/cc @3cb

<!-- gh-comment-id:424817716 --> @joegrasse commented on GitHub (Sep 26, 2018): While I understand why you are hesitating on how to implement this, I would say that I am not quite a fan of the ```Updater``` interface as presented above. This seems like it causes more of a burden on the users of your library to understand the intricacy of how to use your library. This would cause questions like, "When do I need to use the ```Updater``` interface?", "What functions do I need to use with the Updater interface?", etc. Which in the end I think, would lead to more issues and hand holding. For ease of use, I think the syncing should be done behind the scenes. Now how you do it behind the scenes (Updater, Sync), doesn't really matter to me. /cc @3cb

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

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

@3cb suggests an Updater interface which I'm not sure yet what it would look like. But it could be explored to solve issues like yours. Maybe something like this:

func (a *Application) QueueUpdate(updater func()) Application

Then, whenever your application updates in a separate goroutine, instead of calling functions on Table directly, you do this:

app.QueueUpdate(func() {
    table.Clear()
})

It will then be called right before everything is redrawn (during the call to app.Draw()), in a thread-safe fashion.

I actually like passing functions around better than implementing an interface. I think it's easier for the user. An update function could look like this:

func UpdateTableCell(table *tview.Table, row int, column int, newText string) func() {
	return func () {
		table.GetCell(row, column).
			SetText(newText)
	}
}

I'm not so sure about the QueueUpdate approach though. If you were to store the functions in a slice of functions then range over the slice in app.Draw() it would work for simple use cases but wouldn't be thread-safe across the board. It would only work when all modifications of all the used primitives were initiated by app.Draw(). For example, I have previously gotten this output from the race detector:

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 above changes wouldn't prevent this. It's not a critical problem for what I'm doing but I'm not sure it wouldn't be for another use case. There may be other gotchas like this one lurking out there.

What if we added a channel select block to the main event loop within app.Run():
https://gist.github.com/3cb/faa10a07d2c138b81db56776afbf91cd

That would bring all updates back to the main goroutine and allow us to avoid adding mutexes to all the primitive types. I haven't tried to get this running yet but I will when I get a chance.

<!-- gh-comment-id:424932705 --> @3cb commented on GitHub (Sep 27, 2018): > @3cb suggests an `Updater` interface which I'm not sure yet what it would look like. But it could be explored to solve issues like yours. Maybe something like this: > > ```go > func (a *Application) QueueUpdate(updater func()) Application > ``` > > Then, whenever your application updates in a separate goroutine, instead of calling functions on `Table` directly, you do this: > > ```go > app.QueueUpdate(func() { > table.Clear() > }) > ``` > > It will then be called right before everything is redrawn (during the call to `app.Draw()`), in a thread-safe fashion. I actually like passing functions around better than implementing an interface. I think it's easier for the user. An update function could look like this: ```go func UpdateTableCell(table *tview.Table, row int, column int, newText string) func() { return func () { table.GetCell(row, column). SetText(newText) } } ``` I'm not so sure about the `QueueUpdate` approach though. If you were to store the functions in a slice of functions then range over the slice in `app.Draw()` it would work for simple use cases but wouldn't be thread-safe across the board. It would only work when all modifications of all the used primitives were initiated by `app.Draw()`. For example, I have previously gotten this output from the race detector: ``` 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 above changes wouldn't prevent this. It's not a critical problem for what I'm doing but I'm not sure it wouldn't be for another use case. There may be other gotchas like this one lurking out there. What if we added a channel select block to the main event loop within `app.Run()`: https://gist.github.com/3cb/faa10a07d2c138b81db56776afbf91cd That would bring all updates back to the main goroutine and allow us to avoid adding mutexes to all the primitive types. I haven't tried to get this running yet but I will when I get a chance.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

@rivo commented on GitHub (Oct 10, 2018):

Sorry for replying so late. I haven't forgotten about this but have been rather busy lately.

From a point of view of the developer using this package, I totally agree with @joegrasse that all of this should be handled behind the scenes. Your solution, as implemented in #172, which was also my suggestion, will put the burden on the developer. I'd need to provide clear documentation on how to deal with goroutines in tview.

But apart from the huge amount of boilerplate code this would add, I also can't think of ways to make it waterproof. For example, the structure of a TreeView is defined by a tree of TreeNode objects where each node references its children. When you call CollapseAll() (and some other functions), the tree is traversed and all of its children are accessed and modified. Adding a mutex to TreeNode would not prevent race conditions because it would only affect itself, not its children.

So I'm gravitating towards #172 (still have to review it). Unless there are some better suggestions in the next few days, I'll merge it.

Btw, would #172 also get rid of the race condition you posted in your comment?

<!-- gh-comment-id:428636495 --> @rivo commented on GitHub (Oct 10, 2018): Sorry for replying so late. I haven't forgotten about this but have been rather busy lately. From a point of view of the developer using this package, I totally agree with @joegrasse that all of this should be handled behind the scenes. Your solution, as implemented in #172, which was also my suggestion, will put the burden on the developer. I'd need to provide clear documentation on how to deal with goroutines in `tview`. But apart from the huge amount of boilerplate code this would add, I also can't think of ways to make it waterproof. For example, the structure of a [`TreeView`](https://godoc.org/github.com/rivo/tview#TreeView) is defined by a tree of [`TreeNode`](https://godoc.org/github.com/rivo/tview#TreeNode) objects where each node references its children. When you call [`CollapseAll()`](https://godoc.org/github.com/rivo/tview#TreeNode.CollapseAll) (and some other functions), the tree is traversed and all of its children are accessed and modified. Adding a mutex to `TreeNode` would not prevent race conditions because it would only affect itself, not its children. So I'm gravitating towards #172 (still have to review it). Unless there are some better suggestions in the next few days, I'll merge it. Btw, would #172 also get rid of the race condition you posted in your comment?

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

@3cb commented on GitHub (Oct 10, 2018):

From a point of view of the developer using this package, I totally agree with @joegrasse that all of this should be handled behind the scenes. Your solution, as implemented in #172, which was also my suggestion, will put the burden on the developer. I'd need to provide clear documentation on how to deal with goroutines in tview.

But apart from the huge amount of boilerplate code this would add, I also can't think of ways to make it waterproof. For example, the structure of a TreeView is defined by a tree of TreeNode objects where each node references its children. When you call CollapseAll() (and some other functions), the tree is traversed and all of its children are accessed and modified. Adding a mutex to TreeNode would not prevent race conditions because it would only affect itself, not its children.

I agree with all that. I originally fixed my own data races by adding mutexes where I needed them but it became more complicated than I realized because I had to add them to TableCell as well as Table. #172 definitely requires more thought by the user but using closures and calling app.QueueUpdate isn't horrible. I'd be happy to help with the documentation.

So I'm gravitating towards #172 (still have to review it). Unless there are some better suggestions in the next few days, I'll merge it.

Btw, would #172 also get rid of the race condition you posted in your comment?

Yes. I've run it with the race detector and haven't gotten any warnings. It would be helpful if others tested it also.

If you decide against #172 and go the other route, I can help with that too.

<!-- gh-comment-id:428658162 --> @3cb commented on GitHub (Oct 10, 2018): > From a point of view of the developer using this package, I totally agree with @joegrasse that all of this should be handled behind the scenes. Your solution, as implemented in #172, which was also my suggestion, will put the burden on the developer. I'd need to provide clear documentation on how to deal with goroutines in `tview`. > > But apart from the huge amount of boilerplate code this would add, I also can't think of ways to make it waterproof. For example, the structure of a [`TreeView`](https://godoc.org/github.com/rivo/tview#TreeView) is defined by a tree of [`TreeNode`](https://godoc.org/github.com/rivo/tview#TreeNode) objects where each node references its children. When you call [`CollapseAll()`](https://godoc.org/github.com/rivo/tview#TreeNode.CollapseAll) (and some other functions), the tree is traversed and all of its children are accessed and modified. Adding a mutex to `TreeNode` would not prevent race conditions because it would only affect itself, not its children. > I agree with all that. I originally fixed my own data races by adding mutexes where I needed them but it became more complicated than I realized because I had to add them to `TableCell` as well as `Table`. #172 definitely requires more thought by the user but using closures and calling `app.QueueUpdate` isn't horrible. I'd be happy to help with the documentation. > So I'm gravitating towards #172 (still have to review it). Unless there are some better suggestions in the next few days, I'll merge it. > > Btw, would #172 also get rid of the race condition you posted in your comment? Yes. I've run it with the race detector and haven't gotten any warnings. It would be helpful if others tested it also. If you decide against #172 and go the other route, I can help with that too.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

@joegrasse commented on GitHub (Oct 11, 2018):

I still believe the behind the scenes fix would be the better solution. I also agree that it would be more difficult to get correct. If you would like help with that I am also willing and able.

<!-- gh-comment-id:428965199 --> @joegrasse commented on GitHub (Oct 11, 2018): I still believe the behind the scenes fix would be the better solution. I also agree that it would be more difficult to get correct. If you would like help with that I am also willing and able.

kerem commented 2026-03-04 01:02:07 +03:00

Author

Owner

Copy link

@rivo commented on GitHub (Oct 12, 2018):

I'm open to suggestions for solutions to the problems mentioned above.

<!-- gh-comment-id:429213059 --> @rivo commented on GitHub (Oct 12, 2018): I'm open to suggestions for solutions to the problems mentioned above.

kerem commented 2026-03-04 01:02:08 +03:00

Author

Owner

Copy link

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

I merged #172. For implementation details, see https://github.com/rivo/tview/pull/172#issuecomment-433702294. For details on how to deal with concurrency in tview, please check out the package documentation.

Even now, after I dove into this topic in detail, I don't see how this could have been made thread-safe in a self-contained way. No primitive stands on its own so a "global" solution was needed. Anyway, using QueueUpdate() should not impose too much overhead on users.

<!-- gh-comment-id:433702554 --> @rivo commented on GitHub (Oct 28, 2018): I merged #172. For implementation details, see https://github.com/rivo/tview/pull/172#issuecomment-433702294. For details on how to deal with concurrency in `tview`, please check out the package documentation. Even now, after I dove into this topic in detail, I don't see how this could have been made thread-safe in a self-contained way. No primitive stands on its own so a "global" solution was needed. Anyway, using `QueueUpdate()` should not impose too much overhead on users.

kerem referenced this issue 2026-03-04 01:02:16 +03:00

[GH-ISSUE #169] Adopt GO 1.11 Modules Versioning #134

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

master

image

revert-759-textarea

v0.42.0

Labels

Clear labels   

announcement

  

on roadmap

  

pull-request

Mirrored from GitHub Pull Request

  

requesting feedback

  

will consider

No labels

announcement

on roadmap

pull-request

requesting feedback

will consider

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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#116

No description provided.