Fix idle wakeup problem (second attempt) #157
Conversation
This commit uses the poller package (github.com/zyedidia/poller which is a slighty modified version of github.com/npat-efault/poller -- the cgo code has been removed) so that the Read properly terminates when Close is called.
|
Thanks! I am going to look at this in more detail but I wont have a chance before Tuesday as I am going to be away from access to tech for a few days. (whitewater rafting)
…Sent from my iPhone
On Jun 16, 2017, at 9:36 AM, Zachary Yedidia ***@***.***> wrote:
I found this package: https://github.com/npat-efault/poller which seems to use poll in some way to fix the issue on MacOS that we were having with the Close and Read.
For example, this code works as expected:
package main
import (
"fmt"
"os"
"time"
"github.com/npat-efault/poller"
)
func main() {
f, e := poller.Open("/dev/tty", poller.O_RW)
if e != nil {
fmt.Println("Cannot open /dev/tty:", e)
os.Exit(1)
}
go func() {
time.Sleep(time.Second * 4)
fmt.Println("Closing...")
f.Close()
fmt.Println("Close finished")
}()
data := make([]byte, 128)
for {
n, e := f.Read(data)
if e != nil {
fmt.Println("Error: ", e)
return
}
fmt.Println("Got", n, "bytes")
}
}
Since it appears that this problem only occurs on Mac, the implementations for Linux, bsd... do not use this library (perhaps bsd is buggy? I have only been able to test on MacOS and Linux). The poller is only used on Mac so now darwin should get its own tscreen file.
I have also set VTIME=0 and VMIN=1 so that tcell doesn't wake up unless there is data to read.
I removed the whole indoneq channel because it was just blocking (I think it was used for syncing the wakeup with the close?) and didn't seem to be of any use.
I also encountered a problem with the Alt keys after this change -- they were only responding on the next keypress. But if expire was always true then it worked well, so I effectively always made expire true by removing that if statement.
Let me know if you see any problems. I think this is a pretty good solution because it allows restarting the library during the program. Tcell in general is also a little snappier as a result (there is no 1/10th second delay when quitting or when use Alt keys).
You can view, comment on, or merge this pull request online at:
#157
Commit Summary
Fix idle wakeup problem
File Changes
M tscreen.go (39)
M tscreen_bsd.go (28)
A tscreen_darwin.go (121)
M tscreen_linux.go (26)
M tscreen_posix.go (12)
Patch Links:
https://github.com/gdamore/tcell/pull/157.patch
https://github.com/gdamore/tcell/pull/157.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Now that read blocks indefinitely, we need it to run in a separate goroutine so that if a sigwinch event is received, we can process it.
|
I noticed there were some problems receiving the window resize events, so I put the input reading in a separate goroutine (since it blocks forever now) so that now there is a select between the quit channel, the sigwinch channel, and the input channel. This PR also has the benefit that it turns Terminal size: Before: After: Terminal size: Before: After: But I'm getting ahead of myself because zyedidia@8ae342e is separate from this PR (although it requires the |
|
I owe you a review, and either a merge or actionable feedback. Stay tuned. |
|
I'm a little less than optimal (2 glasses of wine, at the end of the evening) but I promise to review when fully sober at peak efficiency within 24 hours. Thanks. |
|
FYI I don't think you should merge this yet. There are still some issues that I found but haven't fixed. |
This commit also improves the performance of draw() by buffering the output and sending it all to the terminal at the end of the draw().
Codecov Report
@@ Coverage Diff @@
## master #157 +/- ##
=========================================
Coverage ? 75.97%
=========================================
Files ? 19
Lines ? 5358
Branches ? 0
=========================================
Hits ? 4071
Misses ? 1257
Partials ? 30
Continue to review full report at Codecov.
|
|
Oops did I do something bad? |
|
I've taken a totally different approach, as part of fixing #164 Please have a look at the inputrefactor branch. Turns out this was simpler by just separating the read into its own loop, and adding another "mainloop" goroutine. Most likely I will close this in favor of that branch. |
I found this package: https://github.com/npat-efault/poller which seems to use poll in some way to fix the issue on MacOS that we were having with the Close and Read.
For example, this code works as expected:
The original library used some cgo on all systems other than Linux, so I have made a fork and removed the need for cgo (fork at https://github.com/zyedidia/poller).
Since it appears that this problem only occurs on Mac, the implementations for Linux, bsd... do not use this library (perhaps bsd is buggy? I have only been able to test on MacOS and Linux -- edit: I tested on FreeBSD and it did not exhibit the bug that MacOS does). The poller is only used on Mac so now darwin should get its own tscreen file.
I have also set
VTIME=0andVMIN=1so that tcell doesn't wake up unless there is data to read.I removed the whole
indoneqchannel because it was just blocking (I think it was used for syncing the wakeup with the close?) and didn't seem to be of any use.I also encountered a problem with the Alt keys after this change -- they were only responding on the next keypress. But if
expirewas always true then it worked well, so I effectively always madeexpiretrue by removing that if statement.Let me know if you see any problems. I think this is a pretty good solution because it allows restarting the library during the program. Tcell in general is also a little snappier as a result (there is no 1/10th second delay when quitting or when use Alt keys).
Ref #129