WinGUI Issue with Locking and Non-blocking Input
TurkeyMcMac opened this issue · 15 comments
In some circumstances, input is never received using the WinGUI backend (at least on Wine). Take this code, for example:
#include <curses.h>
#include <windows.h>
int main(void)
{
initscr();
timeout(0);
for (int i = 0; getch() == ERR; ++i) {
mvprintw(0, 0, "%d", i);
refresh();
Sleep(100);
}
endwin();
puts("Done.");
}It is my understanding that, since this program does not block in getch and delays with Sleep rather than napms, the window thread waits forever to enter/lock the critical section and deliver the input to the main thread. I could just use napms, of course, but I don't think doing so should be a requirement.
Could the main thread be changed to only lock PDC_cs when accessing the data shared between threads? Then the window thread could communicate the input without the main thread specifically allowing it to do so.
(I've barely used the Windows API, so I could be completely misunderstanding things.)
"... I could just use napms, of course, but I don't think doing so should be a requirement."
Why not? Taking out the #include <windows.h> and replacing Sleep() with napms causes the code to work (for me, anyway) with WinGUI on Wine, and in VT, and (I'd expect) all other platforms. Use of the Windows-specific function gains you nothing and loses the vaunted portability aspect of Curses/PDCurses.
So I don't see it as a big problem, but I'm wondering if we'd see big problems elsewhere. Perhaps with a Windows call that can't just be conveniently replaced by a drop-in Curses equivalent.
And on further inspection... I'm unclear as to why, at the time you make that Sleep() call, we're still in a critical section. We added all that CriticalSection code as part of commit dd59e4c, to work around ugliness in screen resizing (issue #156). This also fixed a lesser problem in that the program would freeze while resizing or moving the window. Before that commit, everything was safely single-threaded.
I'll have to get back to you on that... not sure if there's a way to do it without reverting to a single-thread system (which wouldn't be so bad).
Yes, this issue probably doesn't come up very often without the napms fix. This was the code (not quite verbatim) where I actually encountered the problem:
#ifdef _WIN32
# include <stdint.h>
struct ticker {
uint32_t last_tick;
uint32_t interval;
};
/* ... */
# include <windows.h>
void ticker_init(struct ticker *tkr, int interval)
{
tkr->last_tick = GetTickCount();
tkr->interval = interval;
}
void tick(struct ticker *tkr)
{
DWORD now = GetTickCount();
DWORD deadline = tkr->last_tick + tkr->interval;
if (now < deadline) {
Sleep(deadline - now);
tkr->last_tick = deadline;
} else {
tkr->last_tick = now;
}
}
#endif /* defined(_WIN32) */ I was using this code to delay. I didn't initially think to use napms because I was already using the Windows API for other reasons.
Just returning to this, while looking for issues that ought to be fixed before a release is made. This may conceivably be linked to issue #212 : a problem in multi-threading in which we (at least appear to be) in a CriticalSection chunk of code more than we ought to be.
I also came across this behavior as I tried to poll for keys in a tight loop as a way to interrupt a long-running operation. You do actually need at least a napms(1) or timeout(1) in order to receive any keys at all with a non-blocking getch(). This is clearly a functional difference to WinCON and other curses variants and might slow down your loop significantly - in my case beyond anything acceptable.
Is there any other workaround to cause key processing periodically in WinGUI? I don't really care if the UI is responsive in my tight loop, only that I can more or less efficiently detect key presses.
As I use CTRL+C as an interruption key, I can actually use a signal handler on ncurses/UNIX and a control handler on PDCurses/WinCON to detect interruptions asynchronously. Perhaps I will try to install a low level keyboard hook on WinGUI to completely bypass PDCurses for CTRL+C-handling...
(Refreshing my memory on this one... starting by compiling and running @TurkeyMcMac's example code that started this issue, and seeing that the misbehavior remains: if you call Sleep() instead of napms(), keys are ignored.)
I should note that my understanding of how the multi-threading is working here is abysmal. I am somewhat tempted to return to a single-threaded system, even though things may look a bit ugly during resizing and will (somewhat inexplicably) freeze when windows are moved. That would basically be a reversion of commit dd59e4c.
Based on some rough efforts of removing EnterCriticalSection()/LeaveCriticalSection() pairs has left me still more confused. It appears that in most cases, you can enter a critical section, but you can never leave.
I have a possible workaround, though. Since we seem to need a PDC_napms(1) anyway, I added a zero-millisecond nap to PDC_check_key() [EDIT: make that "added a call to SwitchToThread()" after @rhaberkorn pointed that out to me] :
bool PDC_check_key(void)
{
extern CRITICAL_SECTION PDC_cs; /* new code starts here... */
LeaveCriticalSection(&PDC_cs);
SwitchToThread();
EnterCriticalSection(&PDC_cs); /* ...and ends here */
if( PDC_key_queue_low != PDC_key_queue_high)
return TRUE;
return FALSE;
}...and that caused the 'test code' to exit properly when a key was hit. (Even with this fix, though, the failure for LeaveCriticalSection() to actually leave the critical section is worrisome to me, and I'm still tempted to go back to a single-thread system.)
If you look at commit dd59e4c, where dual-threading was added, you'll see that at that time, a PDC_napms(1); call was removed. Putting that back in also fixes this problem. But as you note, if your loop is tight enough, that's not a great idea. If, say, you'd normally do 10000 loops/second, PDC_napms(1); would add ten seconds to the run time. The zero-millisecond "nap" will (I think) not do that.
Update : I just checked, and the above trickery does let you get a relatively quick response from getch() in WinGUI. I wrote a small test program to evaluate the speed of a getch() call. The workload for this program is almost entirely just calling getch(). Results, as described within the source code :
DOS (in DOSBox) : 35000/s
WinGUI on Wine : about 15000/s [1.5 million/second if using SwitchToThread()]
WinCon on Wine : about 20000/s
SDL1: 520000/s
SDL2: 260000/s
VT, Linux framebuffer : 1.3 million/s
(getch( ) code for those two is basically the same)
X11: 370000/s
ncurses : 1.5 million/s
Note that this is on my somewhat elderly machine; your results may vary, and I don't have a 'real' Windows or DOS box handy (and it'd be a slower box, so not an apples-to-apples comparison anyway). As expected, shove a napms(1) into the loop, and all platformsl drop to a bit under 1000 iterations/second (and napms(n) brings it down to 1000/n per second.)
You'll note logic in the code to adjust the frequency of screen updates, so that they're done about once a second. I recommend this sort of trickery, both for screen updates and checking for keyboard hits, in any code with a tight inner loop. There is really no point in updating the screen or checking the keyboard more often than, say, twice a second, and if you have a really tight loop, it'll always be easy to spend more time on screen updates and keyboard hits than you do doing actual work. (Of course, in this case, I used said trickery only to avoid wasting time on screen updates, because I was testing getch() speed. Normally, I'd only check getch() every, say, freq/4 iterations.)
While it isn't necessarily a good idea to sleep(0) the win32 only Sleep(0) is fine. So it looks like the win* ports can stay multi-threaded with the change internal to those, correct?
Would this completely fix this issue?
There is really no point in updating the screen or checking the keyboard more often than, say, twice a second, and if you have a really tight loop, it'll always be easy to spend more time on screen updates and keyboard hits than you do doing actual work. (Of course, in this case, I used said trickery only to avoid wasting time on screen updates, because I was testing
getch()speed. Normally, I'd only checkgetch()every, say, freq/4 iterations.)
That's certainly an optimization I still have to experiment with. But anyway, even if you rate-limit the polling, you won't currently receive any keypresses. Do you suggest to add napms(1) every time I call getch() (or configure a timeout(1))?
Is there a Windows function to yield the current thread? Perhaps that would work in place of Sleep(0). I don't know if there would be any benefit though.
Is there a Windows function to yield the current thread?
Yes, there is SwitchToThread().
@rhaberkorn - thank you; that's a better choice. In my above patch for pdckbd.c, replace the call to Sleep( 0); with SwitchToThread(); it's much more responsive, allowing about 1.6 million calls/second to getch() (i.e., puts it in line with the performance on VT, fb, and ncurses.)
I'd thought that Sleep(0); would be nearly instantaneous. I wouldn't call ~60 microseconds a long time, but am happy to knock it to under a microsecond.
But after patching PDC_check_key(), with either Win32 function, the keypresses should be responsive with no change needed in your application... are you finding otherwise?
@GitMensch - yes, this fixes the problem on WinGUI (it's not a WinCon issue; except for beeping, that platform is single-threaded). At least, it fixes the lack of responsiveness on my machine; waiting for confirmation from @rhaberkorn that it's working there as well before committing and pushing to the site.
We then have the question of why WinCon's getch() is so slow. Might be a Wine thing, though.
The workaround seems to do fine. I've hacked it into SciTECO to work with the current MSYS-version of PDCursesMod for the time being.
I am looking forward to see this released upstream and into MSYS.
There is no PDCursesMod in MSYS, so I guess you mean the MSYS2 packages - and yes, those are updated very shortly after a new release (not sure if Bill feels there "is enough" to provide a new minor release).
not sure if Bill feels there "is enough" to provide a new minor release
Since I do have a workaround - albeit a very hacky one - this is not urgent for me.
The workaround seems to do fine
Excellent. Fix committed and pushed to GitHub.