[GH-ISSUE #179] Freeze and Crash #139

New issue

Closed

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

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

Owner

Copy link

Originally created by @aunoor on GitHub (Nov 7, 2018).
Original GitHub issue: https://github.com/rivo/tview/issues/179

Hi!

New ability to send redraw request to app is cool, but adding second event loop is not.

Crush
App crush on exit. Just Ctrl-C and we see panic trace.
It happened because screen is gone in Stop(), but event queue still have tcell.EventResize.

Freeze
Let take Box example and change it a bit:

// Demo code for the Box primitive.
package main

import (
	"github.com/gdamore/tcell"
	"github.com/rivo/tview"
)

func main() {
	box := tview.NewBox().
		SetBorder(true).
		SetBorderAttributes(tcell.AttrBold).
		SetTitle("A title")

	app:=tview.NewApplication()
	app.SetInputCapture(func (event *tcell.EventKey) *tcell.EventKey {
		switch event.Key() {
			case tcell.KeyCtrlC:
			case tcell.KeyCtrlQ:
				app.Stop()
			default:
				return event
		}
		return nil
		})

	if err := app.SetRoot(box, true).Run(); err != nil {
		panic(err)
	}
}

As you can see, I add input handler on app level. It just eat Ctrl-C and allow quit on Ctrl-Q.
Build, run. Ctrl-C - and we going to endless loop somewhere around EventLoop: label
Exit by Ctrl-Q working as expected, but it strange: on my more complex app any call of Application.Stop() cause panic.

Why adding more complicated mechanism if there is already simple, reliable and extendable tcell.PostEvent()?

Originally created by @aunoor on GitHub (Nov 7, 2018). Original GitHub issue: https://github.com/rivo/tview/issues/179 Hi! New ability to send redraw request to app is cool, but adding second event loop is not. **Crush** App crush on exit. Just Ctrl-C and we see panic trace. It happened because screen is gone in Stop(), but event queue still have tcell.EventResize. **Freeze** Let take Box example and change it a bit: ``` // Demo code for the Box primitive. package main import ( "github.com/gdamore/tcell" "github.com/rivo/tview" ) func main() { box := tview.NewBox(). SetBorder(true). SetBorderAttributes(tcell.AttrBold). SetTitle("A title") app:=tview.NewApplication() app.SetInputCapture(func (event *tcell.EventKey) *tcell.EventKey { switch event.Key() { case tcell.KeyCtrlC: case tcell.KeyCtrlQ: app.Stop() default: return event } return nil }) if err := app.SetRoot(box, true).Run(); err != nil { panic(err) } } ``` As you can see, I add input handler on app level. It just eat Ctrl-C and allow quit on Ctrl-Q. Build, run. Ctrl-C - and we going to endless loop somewhere around EventLoop: label Exit by Ctrl-Q working as expected, but it strange: on my more complex app any call of Application.Stop() cause panic. Why adding more complicated mechanism if there is already simple, reliable and extendable tcell.PostEvent()?

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

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

Author

Owner

Copy link

@3cb commented on GitHub (Nov 9, 2018):

As you can see, I add input handler on app level. It just eat Ctrl-C and allow quit on Ctrl-Q.
Build, run. Ctrl-C - and we going to endless loop somewhere around EventLoop: label
Exit by Ctrl-Q working as expected, but it strange: on my more complex app any call of Application.Stop() cause panic.

I think I found the issue with Ctrl-C:
github.com/rivo/tview@61a4cf388a/application.go (L219)

Returning nil from inputCapture breaks the EventLoop rather than continuing to the next iteration. Replacing break EventLoop with continue fixes the above example.

Why adding more complicated mechanism if there is already simple, reliable and extendable tcell.PostEvent()?

This is a good question and I'm not sure why it didn't occur to me when I suggested the current design. We should probably use tcell.PostEventWait() in a separate goroutine so that updates don't get dropped but it should still reduce the complexity. I think it's possible without breaking the API as well. I'll take a shot at it this weekend.

<!-- gh-comment-id:437482571 --> @3cb commented on GitHub (Nov 9, 2018): > As you can see, I add input handler on app level. It just eat Ctrl-C and allow quit on Ctrl-Q. > Build, run. Ctrl-C - and we going to endless loop somewhere around EventLoop: label > Exit by Ctrl-Q working as expected, but it strange: on my more complex app any call of Application.Stop() cause panic. I think I found the issue with Ctrl-C: https://github.com/rivo/tview/blob/61a4cf388aac73e417df122b357b43a5dbe90f41/application.go#L219 Returning `nil` from `inputCapture` breaks the `EventLoop` rather than continuing to the next iteration. Replacing `break EventLoop` with `continue` fixes the above example. > Why adding more complicated mechanism if there is already simple, reliable and extendable tcell.PostEvent()? This is a good question and I'm not sure why it didn't occur to me when I suggested the current design. We should probably use `tcell.PostEventWait()` in a separate goroutine so that updates don't get dropped but it should still reduce the complexity. I think it's possible without breaking the API as well. I'll take a shot at it this weekend.

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

Author

Owner

Copy link

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

Thanks for catching this. Yes, the break was wrong. I fixed that.

We should probably use tcell.PostEventWait() in a separate goroutine so that updates don't get dropped but it should still reduce the complexity.

How does using tcell.PostEventWait() help with syncing updates and events? It's probably better to queue events using this function than to post them to our own channel (and since tcell.PollEvent() is already in its own goroutine, we probably don't need another goroutine for tcell.PostEventWait()). But we still have to make sure that updates don't interfere with events. So I would think most of what we did here will remain the same.

Or is there something I'm not seeing here?

<!-- gh-comment-id:437573487 --> @rivo commented on GitHub (Nov 10, 2018): Thanks for catching this. Yes, the `break` was wrong. I fixed that. > We should probably use tcell.PostEventWait() in a separate goroutine so that updates don't get dropped but it should still reduce the complexity. How does using `tcell.PostEventWait()` help with syncing updates and events? It's probably better to queue events using this function than to post them to our own channel (and since `tcell.PollEvent()` is already in its own goroutine, we probably don't need another goroutine for `tcell.PostEventWait()`). But we still have to make sure that updates don't interfere with events. So I would think most of what we did here will remain the same. Or is there something I'm not seeing here?

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

Author

Owner

Copy link

@aunoor commented on GitHub (Nov 10, 2018):

After update to last master my example start to generate next panic trace:

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=0x18 pc=0x523ef8]

goroutine 1 [running]:
github.com/rivo/tview.(*Application).Run.func1(0x12c94050)
        C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:150 +0x6b
panic(0x54dc40, 0x6696f0)
        C:/Go/src/runtime/panic.go:513 +0x164
github.com/rivo/tview.(*Application).Run(0x12c94050, 0x0, 0x0)
        C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:241 +0x218
main.main()
        C:/Users/aunoo/go/src/aunoor/box/main.go:33 +0x195

On first glance it look simple, and I change

screen.Clear()
a.draw()

to

if screen != nil {
  screen.Clear()
  a.draw()
}

and got next:

fatal error: all goroutines are asleep - deadlock!

goroutine 1 [select]:
github.com/rivo/tview.(*Application).Run(0x13094050, 0x0, 0x0)
        C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:202 +0x1ad
main.main()
        C:/Users/aunoo/go/src/aunoor/box/main.go:33 +0x195

goroutine 20 [chan receive]:
github.com/rivo/tview.(*Application).Run.func2(0x13104af0, 0x13094050)
        C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:164 +0xf1
created by github.com/rivo/tview.(*Application).Run
        C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:162 +0x12b

May be I do something wrong?

OS: Windows 10
Go: go version go1.11.1 windows/386

<!-- gh-comment-id:437592391 --> @aunoor commented on GitHub (Nov 10, 2018): After update to last master my example start to generate next panic trace: ``` 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=0x18 pc=0x523ef8] goroutine 1 [running]: github.com/rivo/tview.(*Application).Run.func1(0x12c94050) C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:150 +0x6b panic(0x54dc40, 0x6696f0) C:/Go/src/runtime/panic.go:513 +0x164 github.com/rivo/tview.(*Application).Run(0x12c94050, 0x0, 0x0) C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:241 +0x218 main.main() C:/Users/aunoo/go/src/aunoor/box/main.go:33 +0x195 ``` On first glance it look simple, and I change ``` screen.Clear() a.draw() ``` to ``` if screen != nil { screen.Clear() a.draw() } ``` and got next: ``` fatal error: all goroutines are asleep - deadlock! goroutine 1 [select]: github.com/rivo/tview.(*Application).Run(0x13094050, 0x0, 0x0) C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:202 +0x1ad main.main() C:/Users/aunoo/go/src/aunoor/box/main.go:33 +0x195 goroutine 20 [chan receive]: github.com/rivo/tview.(*Application).Run.func2(0x13104af0, 0x13094050) C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:164 +0xf1 created by github.com/rivo/tview.(*Application).Run C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:162 +0x12b ``` May be I do something wrong? OS: Windows 10 Go: go version go1.11.1 windows/386

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

Author

Owner

Copy link

@3cb commented on GitHub (Nov 12, 2018):

screen.Clear()
a.draw()

to

if screen != nil {
  screen.Clear()
  a.draw()
}

What happens when you do the following instead?

case *tcell.EventResize:
	a.Lock()
	screen := a.screen
	screen.Clear()
        a.Unlock()
	a.draw()

<!-- gh-comment-id:438075502 --> @3cb commented on GitHub (Nov 12, 2018): > ``` > screen.Clear() > a.draw() > ``` > to > > ``` > if screen != nil { > screen.Clear() > a.draw() > } > ``` What happens when you do the following instead? ```go case *tcell.EventResize: a.Lock() screen := a.screen screen.Clear() a.Unlock() a.draw() ```

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

Author

Owner

Copy link

@aunoor commented on GitHub (Nov 13, 2018):

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=0x18 pc=0x523edd]

goroutine 1 [running]:
github.com/rivo/tview.(*Application).Run.func1(0x13094050)
        C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:150 +0x6b
panic(0x54dc40, 0x6696f0)
        C:/Go/src/runtime/panic.go:513 +0x164
github.com/rivo/tview.(*Application).Run(0x13094050, 0x0, 0x0)
        C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:240 +0x1fd
main.main()
        C:/Users/aunoo/go/src/aunoor/box/main.go:33 +0x195

application.go:240 is screen.Clear()

<!-- gh-comment-id:438218553 --> @aunoor commented on GitHub (Nov 13, 2018): ``` 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=0x18 pc=0x523edd] goroutine 1 [running]: github.com/rivo/tview.(*Application).Run.func1(0x13094050) C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:150 +0x6b panic(0x54dc40, 0x6696f0) C:/Go/src/runtime/panic.go:513 +0x164 github.com/rivo/tview.(*Application).Run(0x13094050, 0x0, 0x0) C:/Users/aunoo/go/src/github.com/rivo/tview/application.go:240 +0x1fd main.main() C:/Users/aunoo/go/src/aunoor/box/main.go:33 +0x195 ``` application.go:240 is `screen.Clear()`

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

Author

Owner

Copy link

@3cb commented on GitHub (Nov 13, 2018):

This happens when you call Application.Stop() directly from another go routine?

<!-- gh-comment-id:438323289 --> @3cb commented on GitHub (Nov 13, 2018): This happens when you call `Application.Stop()` directly from another go routine?

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

Author

Owner

Copy link

@aunoor commented on GitHub (Nov 13, 2018):

This happens when you call Application.Stop() directly from another go routine?

No, as you can see in example from my first post, Application.Stop() called from main thread, inside inputCapture function. Moreover, unmodified Box example also panic on Ctrl-C.

Windows: panic
Fedora linux, go version go1.10.4 linux/386 in system console (no X): all ok, no panic

<!-- gh-comment-id:438357729 --> @aunoor commented on GitHub (Nov 13, 2018): > This happens when you call `Application.Stop()` directly from another go routine? No, as you can see in example from my first post, `Application.Stop()` called from main thread, inside inputCapture function. Moreover, unmodified Box example also panic on Ctrl-C. Windows: panic Fedora linux, go version go1.10.4 linux/386 in system console (no X): all ok, no panic

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

Author

Owner

Copy link

@aunoor commented on GitHub (Nov 13, 2018):

Small update: OpenSuse 15.1 under WSL, go version go1.9.7 linux/amd64: no panic.

<!-- gh-comment-id:438377413 --> @aunoor commented on GitHub (Nov 13, 2018): Small update: OpenSuse 15.1 under WSL, go version go1.9.7 linux/amd64: no panic.

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

Author

Owner

Copy link

@rivo commented on GitHub (Nov 19, 2018):

On Windows, it appears that upon calling Screen.Fini(), tcell sends a "resize" event to the event queue before sending nil whereas on other platforms, that "resize" event is not sent. (@gdamore: Not sure if this is intentional.)

I fixed the code to deal with this. With your example, I'm not getting a deadlock anymore on Windows.

<!-- gh-comment-id:439830048 --> @rivo commented on GitHub (Nov 19, 2018): On Windows, it appears that upon calling `Screen.Fini()`, `tcell` sends a "resize" event to the event queue before sending `nil` whereas on other platforms, that "resize" event is not sent. (@gdamore: Not sure if this is intentional.) I fixed the code to deal with this. With your example, I'm not getting a deadlock anymore on Windows.

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

Author

Owner

Copy link

@3cb commented on GitHub (Nov 19, 2018):

I think this fix is causing unpredictable behavior when using app.Suspend().

Given the following code, sometimes hitting keys sometimes prevents Suspend from returning and allowing the box to be redrawn; the application will just close:

func main() {
	box := tview.NewBox().SetBorder(true)
	app := tview.NewApplication()
	go func() {
		time.Sleep(3 * time.Second)
		app.Suspend(func() {
			println("inside suspend")
			time.Sleep(3 * time.Second)
		})
	}()
	if err := app.SetRoot(box, true).Run(); err != nil {
		panic(err)
	}
}

Again, it doesn't happen every time but something seems off.

<!-- gh-comment-id:440020129 --> @3cb commented on GitHub (Nov 19, 2018): I think this fix is causing unpredictable behavior when using `app.Suspend()`. Given the following code, sometimes hitting keys sometimes prevents `Suspend` from returning and allowing the box to be redrawn; the application will just close: ```go func main() { box := tview.NewBox().SetBorder(true) app := tview.NewApplication() go func() { time.Sleep(3 * time.Second) app.Suspend(func() { println("inside suspend") time.Sleep(3 * time.Second) }) }() if err := app.SetRoot(box, true).Run(); err != nil { panic(err) } } ``` Again, it doesn't happen every time but something seems off.

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

Author

Owner

Copy link

@gdamore commented on GitHub (Nov 19, 2018):

@rivo I don't recall that it was intentional. I'll have to look.

<!-- gh-comment-id:440043300 --> @gdamore commented on GitHub (Nov 19, 2018): @rivo I don't recall that it was intentional. I'll have to look.

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

Author

Owner

Copy link

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

@3cb, thanks. This whole thing just became too complex. I rearranged the code somewhat so it can also deal with the extra resize event on Windows even during suspended mode.

My tests (regular and suspended mode) didn't show any problems on Windows and on macOS. If you catch anything, let me know.

<!-- gh-comment-id:441601682 --> @rivo commented on GitHub (Nov 26, 2018): @3cb, thanks. This whole thing just became too complex. I rearranged the code somewhat so it can also deal with the extra resize event on Windows even during suspended mode. My tests (regular and suspended mode) didn't show any problems on Windows and on macOS. If you catch anything, let me know.

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

Author

Owner

Copy link

@3cb commented on GitHub (Nov 28, 2018):

@rivo, No problem. I took some time to explore using screen.PostEvent() like I mentioned above. I had to add some private types to get tview-specific events(updates, suspend, stop) to implement the tcell.Event interface but I think it makes the flow of events into the main event loop easier to reason about. Hopefully, I will find some time tomorrow to submit a PR just so you can take a look to see if you think it is worthwhile to pursue.

<!-- gh-comment-id:442609340 --> @3cb commented on GitHub (Nov 28, 2018): @rivo, No problem. I took some time to explore using `screen.PostEvent()` like I mentioned above. I had to add some private types to get tview-specific events(updates, suspend, stop) to implement the `tcell.Event` interface but I think it makes the flow of events into the main event loop easier to reason about. Hopefully, I will find some time tomorrow to submit a PR just so you can take a look to see if you think it is worthwhile to pursue.

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

Author

Owner

Copy link

@rivo commented on GitHub (Dec 3, 2018):

It looks like you didn't start with the recent modifications I made. Your PR re-introduces old code and variables which I had already removed in favour of simplification. I'm not planning on going back to the old version, mostly because it had bugs that caused issues on Windows, but also because it was unnecessarily complex.

If you'd like to submit changes, I suggest you start with the current codebase as the code has changed significantly since we started this discussion.

I have to say I'm reasonably happy with the current Application code. (With the disclaimer that I haven't understood yet how Screen.PostEvent() can help us with updates in goroutines. Maybe there's something I'm not seeing at the moment.)

<!-- gh-comment-id:443655405 --> @rivo commented on GitHub (Dec 3, 2018): It looks like you didn't start with the recent modifications I made. Your PR re-introduces old code and variables which I had already removed in favour of simplification. I'm not planning on going back to the old version, mostly because it had bugs that caused issues on Windows, but also because it was unnecessarily complex. If you'd like to submit changes, I suggest you start with the current codebase as the code has changed significantly since we started this discussion. I have to say I'm reasonably happy with the current `Application` code. (With the disclaimer that I haven't understood yet how `Screen.PostEvent()` can help us with updates in goroutines. Maybe there's something I'm not seeing at the moment.)

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

Author

Owner

Copy link

@3cb commented on GitHub (Dec 3, 2018):

Sorry about that. I don't think I submitted a PR. All I did was push to my fork of the repo and then delete it; didn't know it would automatically create a notification in this issue.

As far as using PostEvent(): What you did with the screenReplacement channel requires less code so I decided not to submit a PR. I did, however, realize there is potential for deadlock if app.QueueUpdate is called from inputCapture. So I'll open a new issue for that.

<!-- gh-comment-id:443718213 --> @3cb commented on GitHub (Dec 3, 2018): Sorry about that. I don't think I submitted a PR. All I did was push to my fork of the repo and then delete it; didn't know it would automatically create a notification in this issue. As far as using `PostEvent()`: What you did with the `screenReplacement` channel requires less code so I decided not to submit a PR. I did, however, realize there is potential for deadlock if `app.QueueUpdate` is called from `inputCapture`. So I'll open a new issue for that.

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

Author

Owner

Copy link

@rivo commented on GitHub (Dec 3, 2018):

Ok, that was a misunderstanding then. I thought it was a new submission.

<!-- gh-comment-id:443771501 --> @rivo commented on GitHub (Dec 3, 2018): Ok, that was a misunderstanding then. I thought it was a new submission.

kerem referenced this issue 2026-03-04 01:08:17 +03:00

[PR #139] [MERGED] Replace all instances of ANSII #863

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

No description provided.