Table: Fix changing Rows during runtime

[rck]

This fixes a concurrency problem in termui.Table. This was the part I cared most about, but maybe this is also a starting point if these kind of problems should get fixed deeper down in the core or per termui module.

Original commit message:

So far changing table.Rows had two problems that made it panic in dynamic environments, where users tried to reassign new table.Rows.

• A trivial case where the Colors array was accessed out of bounds because the index was generated by iterating over table.Rows (Analysis()). While nasty, this could be fixed by the user by also adding Colors elements to the according Color arrays.

• An internal race in Buffer() when the table.Rows (even with additional Colors) were updated between "rowCells" calculation and then iterating a few lines below again over the now longer table.Rows. This caused panics when rowCells out of bound got accessed.

This introduces a sync.Mutex and a setter method. The implementation of public methods that are also used in Buffer() got split out to a private implementation (e.g. Analysis -> analysis). The public implementation now does the locking and calls the private implementation. This allows us to take the lock once in Buffer() and then call multiple internal implementations.

The setter (i.e., SetRows()) also takes care of allocating/initializing potentially new Color elements. As commented in the source code, currently not used Color elements are kept as cache without immediately deallocating them.

[rck]

Hi guys, any updates on that?

Am I using the table wrong and you think the number of rows has to be fixed and a public member (Rows) is not allowed to be touched? Do you need an example that this actually happens without the patches?

Regards, rck

[maxdec]

I'm facing that issue too. It's annoying for a long-running dashboard.

[rck]

@maxdec : did you try the patched version? I use LINBIT/termui for a project, basically also a long-running, dynamic dashboard, and have not had any problems since.

Somehow strange that this simply gets ignored...

[maxdec]

Yep I'm using that fork and it works great. I'm no Go expert but the changes make sense to me. (Maybe it needs specs?)

[toby1991]

Facing the same issue

[cjbassi]

Hi, thanks for this PR! So most of termui was reworked in 958a285, and I think this issue has been taken care of since then. Table just has a Draw (previously Buffer) function now that renders the widget and the helper functions have been removed. The only time you should get a race condition with any widgets now is if you have 2 goroutines: 1 for rendering widgets, and 1 for updating any widgets, in which case you have to have a mutex.

Of course, if you run into issues still or notice something missing, feel free to create a new issue or comment here.

Also, if you're curious, the way I've handled race conditions in gotop is by having a loop in main that renders the widgets every second (check out the termui rework/examples for how to do this now) and locks a RWMutex for writing (so it has a unique lock on it) when it renders, and then I have a goroutine for each widget that updates the widget periodically and locks the RWMutex for reading (so it has a shared lock) when it updates. This has been working well for me, but let me know if there are any of ways of doing this.