Block main event loop with mutex during Suspend() call#135
Conversation
kvj
force-pushed
the
suspend-in-goroutine
branch
from
dcfc6de to
2ac0e9e
Compare
rivo
left a comment
rivo
left a comment
There was a problem hiding this comment.
Thanks for the PR. I gave a few comments and requested a few changes.
|
|
||
| // If this value is true, the application has entered suspended mode. | ||
| suspended bool | ||
| // Used for events loop lock during Suspend() |
There was a problem hiding this comment.
Maybe a comment such as Halts the loop during suspended mode . is better here.
|
|
||
| // Wait for next event. | ||
| event := a.screen.PollEvent() | ||
| a.suspendMutex.Unlock() |
There was a problem hiding this comment.
Doesn't it make sense to wrap the entire for block with the lock? It seems to me that the code after this Unlock() could also potentially interfere with suspended mode.
There was a problem hiding this comment.
No, in this case there will be a deadlock, if Suspend() will be called from inputCapture() handler (same goroutine)
|
|
||
| if a.suspended || a.screen == nil { | ||
| if a.screen == nil { | ||
| // Application is already suspended. |
There was a problem hiding this comment.
This comment is now not correct anymore. It should probably read Screen has not yet been initialized..
| var err error | ||
| a.screen, err = tcell.NewScreen() | ||
| if err != nil { | ||
| a.suspendMutex.Unlock() |
There was a problem hiding this comment.
Instead of putting Unlock()'s everywhere, a defer Unlock() at the top is probably better.
|
@rivo thanks for the feedback. I've updated PR |
2 participants
Fixes #134
Please take a look on it.
This replaces
suspended boolflag with dedicated mutex, which simplifies code and fixes above mentioned bug