From 0a0db94084dfe181108c18508ebd312f12d331fb Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 15 Sep 2017 07:17:52 +0100 Subject: [PATCH] Fix cScreen.Fini() race condition (#158) (#159) * Fix cScreen.Fini() race condition Closes gdamore/tcell#158 * Tidy up Windows magic, and fix event ordering * Move a couple of magic values into constants * Add more explanatory comments * Fix ordering WaitForMultipleObjects to give cancellation priority * Also correct some indentation and minor formatting stuff. * Use `chan struct{}` for `scandone` to not deadlock --- console_win.go | 168 ++++++++++++++++++++++++++++++------------------- 1 file changed, 103 insertions(+), 65 deletions(-) diff --git a/console_win.go b/console_win.go index 8337c70..e27bf6a 100644 --- a/console_win.go +++ b/console_win.go @@ -21,11 +21,14 @@ import ( "syscall" "unicode/utf16" "unsafe" + "errors" ) type cScreen struct { in syscall.Handle out syscall.Handle + cancelflag syscall.Handle + scandone chan struct{} evch chan Event quit chan struct{} curx int @@ -95,11 +98,11 @@ var k32 = syscall.NewLazyDLL("kernel32.dll") // Note that Windows appends some functions with W to indicate that wide // characters (Unicode) are in use. The documentation refers to them // without this suffix, as the resolution is made via preprocessor. -// We have to bring in the kernel32.dll directly, so we can get access to some -// system calls that the core Go API lacks. - var ( procReadConsoleInput = k32.NewProc("ReadConsoleInputW") + procWaitForMultipleObjects = k32.NewProc("WaitForMultipleObjects") + procCreateEvent = k32.NewProc("CreateEventW") + procSetEvent = k32.NewProc("SetEvent") procGetConsoleCursorInfo = k32.NewProc("GetConsoleCursorInfo") procSetConsoleCursorInfo = k32.NewProc("SetConsoleCursorInfo") procSetConsoleCursorPosition = k32.NewProc("SetConsoleCursorPosition") @@ -113,6 +116,11 @@ var ( procSetConsoleTextAttribute = k32.NewProc("SetConsoleTextAttribute") ) +const ( + w32Infinite = ^uintptr(0) + w32WaitObject0 = uintptr(0) +) + // NewConsoleScreen returns a Screen for the Windows console associated // with the current process. The Screen makes use of the Windows Console // API to display content and read events. @@ -121,9 +129,9 @@ func NewConsoleScreen() (Screen, error) { } func (s *cScreen) Init() error { - s.evch = make(chan Event, 10) s.quit = make(chan struct{}) + s.scandone = make(chan struct{}) in, e := syscall.Open("CONIN$", syscall.O_RDWR, 0) if e != nil { @@ -137,6 +145,16 @@ func (s *cScreen) Init() error { } s.out = out + cf, _, e := procCreateEvent.Call( + uintptr(0), + uintptr(1), + uintptr(0), + uintptr(0)) + if cf == uintptr(0) { + return e + } + s.cancelflag = syscall.Handle(cf) + s.Lock() s.curx = -1 @@ -191,6 +209,10 @@ func (s *cScreen) Fini() { uintptr(s.mapStyle(StyleDefault))) close(s.quit) + procSetEvent.Call(uintptr(s.cancelflag)) + // Block until scanInput returns; this prevents a race condition on Win 8+ + // which causes syscall.Close to block until another keypress is read. + <-s.scandone syscall.Close(s.in) syscall.Close(s.out) } @@ -496,79 +518,101 @@ func mrec2btns(mbtns, flags uint32) ButtonMask { } func (s *cScreen) getConsoleInput() error { - rec := &inputRecord{} - var nrec int32 - rv, _, er := procReadConsoleInput.Call( - uintptr(s.in), - uintptr(unsafe.Pointer(rec)), - uintptr(1), - uintptr(unsafe.Pointer(&nrec))) - if rv == 0 { - return er - } - if nrec != 1 { - return nil - } - switch rec.typ { - case keyEvent: - krec := &keyRecord{} - krec.isdown = geti32(rec.data[0:]) - krec.repeat = getu16(rec.data[4:]) - krec.kcode = getu16(rec.data[6:]) - krec.scode = getu16(rec.data[8:]) - krec.ch = getu16(rec.data[10:]) - krec.mod = getu32(rec.data[12:]) + // cancelFlag comes first as WaitForMultipleObjects returns the lowest index + // in the event that both events are signalled. + waitObjects := []syscall.Handle{s.cancelflag, s.in} + // As arrays are contiguous in memory, a pointer to the first object is the + // same as a pointer to the array itself. + pWaitObjects := unsafe.Pointer(&waitObjects[0]) - if krec.isdown == 0 || krec.repeat < 1 { - // its a key release event, ignore it + rv, _, er := procWaitForMultipleObjects.Call( + uintptr(len(waitObjects)), + uintptr(pWaitObjects), + uintptr(0), + w32Infinite) + // WaitForMultipleObjects returns WAIT_OBJECT_0 + the index. + switch rv { + case w32WaitObject0: // s.cancelFlag + return errors.New("cancelled") + case w32WaitObject0 + 1: // s.in + rec := &inputRecord{} + var nrec int32 + rv, _, er := procReadConsoleInput.Call( + uintptr(s.in), + uintptr(unsafe.Pointer(rec)), + uintptr(1), + uintptr(unsafe.Pointer(&nrec))) + if rv == 0 { + return er + } + if nrec != 1 { return nil } - if krec.ch != 0 { - // synthesized key code + switch rec.typ { + case keyEvent: + krec := &keyRecord{} + krec.isdown = geti32(rec.data[0:]) + krec.repeat = getu16(rec.data[4:]) + krec.kcode = getu16(rec.data[6:]) + krec.scode = getu16(rec.data[8:]) + krec.ch = getu16(rec.data[10:]) + krec.mod = getu32(rec.data[12:]) + + if krec.isdown == 0 || krec.repeat < 1 { + // its a key release event, ignore it + return nil + } + if krec.ch != 0 { + // synthesized key code + for krec.repeat > 0 { + s.PostEvent(NewEventKey(KeyRune, rune(krec.ch), + mod2mask(krec.mod))) + krec.repeat-- + } + return nil + } + key := KeyNUL // impossible on Windows + ok := false + if key, ok = vkKeys[krec.kcode]; !ok { + return nil + } for krec.repeat > 0 { - s.PostEvent(NewEventKey(KeyRune, rune(krec.ch), + s.PostEvent(NewEventKey(key, rune(krec.ch), mod2mask(krec.mod))) krec.repeat-- } - return nil - } - key := KeyNUL // impossible on Windows - ok := false - if key, ok = vkKeys[krec.kcode]; !ok { - return nil - } - for krec.repeat > 0 { - s.PostEvent(NewEventKey(key, rune(krec.ch), - mod2mask(krec.mod))) - krec.repeat-- - } - case mouseEvent: - var mrec mouseRecord - mrec.x = geti16(rec.data[0:]) - mrec.y = geti16(rec.data[2:]) - mrec.btns = getu32(rec.data[4:]) - mrec.mod = getu32(rec.data[8:]) - mrec.flags = getu32(rec.data[12:]) - btns := mrec2btns(mrec.btns, mrec.flags) - // we ignore double click, events are delivered normally - s.PostEvent(NewEventMouse(int(mrec.x), int(mrec.y), btns, - mod2mask(mrec.mod))) + case mouseEvent: + var mrec mouseRecord + mrec.x = geti16(rec.data[0:]) + mrec.y = geti16(rec.data[2:]) + mrec.btns = getu32(rec.data[4:]) + mrec.mod = getu32(rec.data[8:]) + mrec.flags = getu32(rec.data[12:]) + btns := mrec2btns(mrec.btns, mrec.flags) + // we ignore double click, events are delivered normally + s.PostEvent(NewEventMouse(int(mrec.x), int(mrec.y), btns, + mod2mask(mrec.mod))) - case resizeEvent: - var rrec resizeRecord - rrec.x = geti16(rec.data[0:]) - rrec.y = geti16(rec.data[2:]) - s.PostEvent(NewEventResize(int(rrec.x), int(rrec.y))) + case resizeEvent: + var rrec resizeRecord + rrec.x = geti16(rec.data[0:]) + rrec.y = geti16(rec.data[2:]) + s.PostEvent(NewEventResize(int(rrec.x), int(rrec.y))) + default: + } default: + return er } + return nil } func (s *cScreen) scanInput() { for { if e := s.getConsoleInput(); e != nil { + close(s.scandone) return } } @@ -600,7 +644,6 @@ var vgaColors = map[Color]uint16{ // Windows uses RGB signals func mapColor2RGB(c Color) uint16 { - winLock.Lock() if v, ok := winColors[c]; ok { c = v @@ -654,7 +697,6 @@ func (s *cScreen) mapStyle(style Style) uint16 { } func (s *cScreen) SetCell(x, y int, style Style, ch ...rune) { - if len(ch) > 0 { s.SetContent(x, y, ch[0], ch[1:], style) } else { @@ -707,7 +749,6 @@ func (s *cScreen) draw() { for y := 0; y < int(s.h); y++ { for x := 0; x < int(s.w); x++ { - mainc, combc, style, width := s.cells.GetContent(x, y) dirty := s.cells.Dirty(x, y) if style == StyleDefault { @@ -811,7 +852,6 @@ func (s *cScreen) setBufferSize(x, y int) { } func (s *cScreen) Size() (int, int) { - s.Lock() w, h := s.w, s.h s.Unlock() @@ -820,7 +860,6 @@ func (s *cScreen) Size() (int, int) { } func (s *cScreen) resize() { - info := consoleInfo{} s.getConsoleInfo(&info) @@ -947,7 +986,6 @@ func (s *cScreen) HasMouse() bool { func (s *cScreen) Resize(int, int, int, int) {} func (s *cScreen) HasKey(k Key) bool { - // Microsoft has codes for some keys, but they are unusual, // so we don't include them. We include all the typical // 101, 105 key layout keys.