Fix cScreen.Fini() race condition (#158) #159

mikesmiffy128 · 2017-07-03T22:31:48Z

Closes #158. Also ought to solve rprichard/winpty#119 and junegunn/fzf#962, and maybe some other issues too.

mikesmiffy128 · 2017-07-03T22:33:06Z

Wait a minute, I left in something stupid.

mikesmiffy128 · 2017-07-03T23:03:32Z

Okay, now it should be good to merge. I accidentally left in a print statement but I just force-pushed it away (after some fumbling with Git 😛).
Edit: that doesn't work either... one moment
Edit 2: NOW it works I promise

Closes gdamore#158

gdamore · 2017-07-04T04:31:46Z

I will be reviewing this change shortly. It seems like a bit of a deep and windows-specific issue (and I'm not really a Windows guy), so I want to make sure I fully understand before I merge. Please bear with me, and thank you for your efforts here.

gdamore · 2017-07-04T04:40:26Z

console_win.go

+
+	rv, _, er := procWaitForMultipleObjects.Call(
+		uintptr(len(waitObjects)),
+		uintptr(unsafe.Pointer(&waitObjects[0])),


This logic is kind of subtle. To be honest I'd really prefer to have some comment here. We're doing some casting to assume addresses of arrays -- this is normal in C, but looks really unusual in Go, and Go programmers may find this hard to grok. I guess a comment like this:

// The address of the first element in the array is also the address of the array.

I'm not sure if that helps a lot, but it might.

gdamore · 2017-07-04T04:42:52Z

console_win.go

+		uintptr(len(waitObjects)),
+		uintptr(unsafe.Pointer(&waitObjects[0])),
+		uintptr(0),
+		^uintptr(0))


This is really strange. I understand what you're doing, which is to try to get to INFINITE, which is -1. Would it be clearer to set INFINITE to some variable - say uintptr(-1) ?

This stuff is kind of gnarly, but anything we can do to make it a little less dense may help future generations -- or just future me. :-)

gdamore · 2017-07-04T04:45:19Z

console_win.go

+		uintptr(0),
+		^uintptr(0))
+	switch rv {
+	case 0: // s.in


So technically, the win32 API doesn't call these 0, 1, etc. Instead they say relative to WAIT_OBJECT_0. I presume you've verified that WAIT_OBJECT_0 is actually 0? Again, I'm not a Win32 expert here, so I just use the Microsoft API documentation. Some commentary here might be helpful.

So also reading the documentation, I don't think the switch statement is entirely correct. Specifically the MS documentation says "If more than one object became signaled during the call, this is the array index of the signaled object with the smallest index value of all the signaled objects." I read that to mean that its possible for more than one object to be signaled. So we could have another even more subtle race here.

I'm not sure offhand how we can test whether a specific event is signaled? I think WaitForSingleObject() with a timeout of zero will allow us to query the single event objects state? Since we are level triggered, this should be safe to use. (Because you selected manual reset.)

gdamore · 2017-07-04T04:53:21Z

console_win.go

+				}
+				return nil
+			}
+			key := KeyNUL // impossible on Windows


What purpose does this assignment serve?

It looks like you're just trying to create the key variable in scope?

gdamore

You've done some good work here, so let me start by saying thanks for that!

I think there's some opportunity to improve the comments a bit to make some fairly ugly code a little easier to grok (ugly because Windows, not your fault, but lets try to help future maintainers).

The other concern I have is about a possible race when multiple objects are signaled -- maybe its a non issue (if we handle only one of the items will the next iteration basically get signaled to deal with the other?) This is super windows-specific, so I'd really like your input here -- if the race is a non-issue because we will be called again for subsequent events then I would like comments to explain that. If that's not the case, then we need to check for each possible event I suppose -- perhaps with WaitForSingleEvent with a zero timeout?

Again, I think we're close on this. If we can wrap up my final concerns, I think we can get this integrated shortly. Thanks for your patience.

mikesmiffy128 · 2017-07-04T11:36:42Z

Hi, thanks for reviewing.

Sorry some of the code is a bit ugly - that actually is probably my fault because I'm new to Go. Regarding magic numbers, I did of course check the actual definitions, however I should probably move those into some constants.

As for the possibile race between the two events... Having thought about it, I think it should probably still work, but if there are numerous unprocessed key events then it could be a while before the cancellation event is received. That said, it would make more sense to swap the order around so that cancellation actually cancels reading instead of waiting for it to finish.

There is also another issue which I thought of after writing this initial version, which is that attempting to write to the scandone channel after an error occurs might cause a lockup if Fini isn't there to read from it. I'm not sure what the intended behavior is if scanInput exits early - does it just stop sending inputs? Should that be somehow handled better too?

Lastly, I'm also not sure what purpose that assignment serves; I'm pretty sure it's just showing up in the diff because of indentation. I didn't knowingly change any of the key handling stuff, unless I did something stupid - which I'll check along with everything else hopefully later today ☺️

* 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.

mikesmiffy128 · 2017-07-04T13:16:02Z

Just pushed some changes, which I think should address most of the review comments. Please let me know if anything else looks wrong. 😛

gdamore · 2017-09-06T06:21:27Z

Sorry running behind things lately (different open source project sucking up all my time), but I'll get to this shortly. Please ping me if you haven't heard back within 72 hours.

mikesmiffy128 · 2017-09-12T13:57:19Z

Hi, it's been another 6 days. I don't want to bother you if you're busy, although it would be nice to get this merged if you do get a moment. 😄

gdamore · 2017-09-15T06:17:43Z

I will squash & merge.

mikesmiffy128 · 2017-09-15T13:29:10Z

Ah, brilliant! Thanks for merging! Also thanks @rprichard for figuring out the bug in the first place 👍

- Close #1169 - gdamore/tcell#159

mikesmiffy128 force-pushed the fix-158 branch 2 times, most recently from 0d67b38 to fd074f2 Compare July 3, 2017 23:01

mikesmiffy128 mentioned this pull request Jul 3, 2017

Allow forcing use of Neovim terminal emulation via g:fzf_use_term junegunn/fzf#962

Closed

mikesmiffy128 force-pushed the fix-158 branch from fd074f2 to da0ba4a Compare July 3, 2017 23:17

Fix cScreen.Fini() race condition

f2d1697

Closes gdamore#158

mikesmiffy128 force-pushed the fix-158 branch from da0ba4a to f2d1697 Compare July 3, 2017 23:20

gdamore reviewed Jul 4, 2017

View reviewed changes

gdamore requested changes Jul 4, 2017

View reviewed changes

Tidy up Windows magic, and fix event ordering

014b005

* 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

5adf181

janlazo mentioned this pull request Aug 17, 2017

Use the terminal by default for nvim-0.2.1 or GVim (nightly) junegunn/fzf#1019

Merged

gdamore approved these changes Sep 15, 2017

View reviewed changes

gdamore merged commit 0a0db94 into gdamore:master Sep 15, 2017

janlazo mentioned this pull request Oct 25, 2017

fzf on gvim windows without WSL? junegunn/fzf#1093

Closed

14 tasks

junegunn mentioned this pull request Dec 3, 2017

fzf doesn't open file in normal mode (gvim/windows) junegunn/fzf#1169

Closed

15 tasks

junegunn added a commit to junegunn/fzf that referenced this pull request Dec 3, 2017

Update tcell to fix double-enter problem on Windows GVim

306d51c

- Close #1169 - gdamore/tcell#159

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cScreen.Fini() race condition (#158) #159

Fix cScreen.Fini() race condition (#158) #159

mikesmiffy128 commented Jul 3, 2017

mikesmiffy128 commented Jul 3, 2017

mikesmiffy128 commented Jul 3, 2017 •

edited

Loading

gdamore commented Jul 4, 2017

gdamore Jul 4, 2017

gdamore Jul 4, 2017

gdamore Jul 4, 2017

gdamore Jul 4, 2017

gdamore Jul 4, 2017

gdamore Jul 4, 2017

gdamore left a comment

mikesmiffy128 commented Jul 4, 2017

mikesmiffy128 commented Jul 4, 2017

gdamore commented Sep 6, 2017

mikesmiffy128 commented Sep 12, 2017

gdamore commented Sep 15, 2017

mikesmiffy128 commented Sep 15, 2017

Fix cScreen.Fini() race condition (#158) #159

Fix cScreen.Fini() race condition (#158) #159

Conversation

mikesmiffy128 commented Jul 3, 2017

mikesmiffy128 commented Jul 3, 2017

mikesmiffy128 commented Jul 3, 2017 • edited Loading

gdamore commented Jul 4, 2017

gdamore Jul 4, 2017

Choose a reason for hiding this comment

gdamore Jul 4, 2017

Choose a reason for hiding this comment

gdamore Jul 4, 2017

Choose a reason for hiding this comment

gdamore Jul 4, 2017

Choose a reason for hiding this comment

gdamore Jul 4, 2017

Choose a reason for hiding this comment

gdamore Jul 4, 2017

Choose a reason for hiding this comment

gdamore left a comment

Choose a reason for hiding this comment

mikesmiffy128 commented Jul 4, 2017

mikesmiffy128 commented Jul 4, 2017

gdamore commented Sep 6, 2017

mikesmiffy128 commented Sep 12, 2017

gdamore commented Sep 15, 2017

mikesmiffy128 commented Sep 15, 2017

mikesmiffy128 commented Jul 3, 2017 •

edited

Loading