On Fri, Jun 15, 2007 at 05:26:41PM -0700, Daniel Burrows <[EMAIL PROTECTED]>
was heard to say:
> So, I need to find a way to do wget_wch() in my main thread, while
> still unblocking when a background thread posts a message to my global
> queue. I guess this means I have to make some sort of dummy
> pipe-to-myself so I can select() on stdin and it (ew ew ew)...except,
> does wget_wch always return on the first character? What about
> multibyte input? The wget_wch manpage is, as usual, silent...
OK, after getting some sleep I realized that it's not that bad.
The attached patch at least fixes the problem I saw in strace,
although I can't reproduce the bug on my dual-core system at the
moment (possibly because I'm running xen vms on it?).
Daniel
Sat Jun 16 09:42:58 PDT 2007 Daniel Burrows <[EMAIL PROTECTED]>
* Rewrite the input thread to eliminate a race condition. (Closes: #414838, #406193)
The problem here is that get_wch() and its friend getch() both implicitly
invoke refresh(). So instead of calling get_wch() from the background
thread, I select() on fd 0, then spawn an event that causes the foreground
thread to run get_wch() and dispatch the result. The tricky bit is that
obviously I don't want to keep selecting and posting events, so I have
synchronization to block the background thread until the event completes.
Sat Jun 16 09:42:32 PDT 2007 Daniel Burrows <[EMAIL PROTECTED]>
* Use the new bootstrap proxy code to simplify the timeout thread.
Sat Jun 16 09:41:53 PDT 2007 Daniel Burrows <[EMAIL PROTECTED]>
* Add utility code in threads:: to make it easier to create proxies for uncopyable thread bootstrap functions.
diff -rN -u old-head/src/generic/util/threads.h new-head/src/generic/util/threads.h
--- old-head/src/generic/util/threads.h 2007-06-16 10:35:46.000000000 -0700
+++ new-head/src/generic/util/threads.h 2007-06-16 10:35:47.000000000 -0700
@@ -1,6 +1,6 @@
// threads.h -*-c++-*-
//
-// Copyright (C) 2005-2006 Daniel Burrows
+// Copyright (C) 2005-2007 Daniel Burrows
//
// This program is free software; you can redistribute it and/or
// modify it under the terms of the GNU General Public License as
@@ -912,6 +912,32 @@
return b.timed_put(in, until);
}
};
+
+ // A utility that proxies for noncopyable thread bootstrap
+ // objects. The only requirement is that the pointer passed
+ // to the constructor must not be destroyed until the thread
+ // completes.
+ template<typename F>
+ class bootstrap_proxy
+ {
+ F *f;
+ public:
+ bootstrap_proxy(F *_f)
+ : f(_f)
+ {
+ }
+
+ void operator()() const
+ {
+ (*f)();
+ }
+ };
+
+ template<typename F>
+ bootstrap_proxy<F> make_bootstrap_proxy(F *f)
+ {
+ return bootstrap_proxy<F>(f);
+ }
}
#endif // THREADS_H
diff -rN -u old-head/src/vscreen/vscreen.cc new-head/src/vscreen/vscreen.cc
--- old-head/src/vscreen/vscreen.cc 2007-06-16 10:35:46.000000000 -0700
+++ new-head/src/vscreen/vscreen.cc 2007-06-16 10:35:46.000000000 -0700
@@ -43,6 +43,7 @@
#include "config/style.h"
#include <generic/util/event_queue.h>
+#include <generic/util/util.h>
#include <generic/util/threads.h>
// For _()
@@ -201,55 +202,135 @@
//////////////////////////////////////////////////////////////////////
// Event management threads
-/** This thread is responsible for posting wget_wch() calls. */
+/** This thread is responsible for posting wget_wch() calls.
+ *
+ * Note that the actual call to wget_wch must take place in the
+ * foreground thread, because wget_wch will invoke wrefresh().
+ * So instead of calling it in the background thread, I post
+ * input events to the foreground thread.
+ *
+ * To prevent the background thread from spamming the foreground
+ * thread with events, I suspend it until the event actually
+ * triggers.
+ */
class input_thread
{
- class key_input_event : public vscreen_event
+ class get_input_event : public vscreen_event
{
- key k;
+ // A reference to the parent's condition mutex;
+ // should be held while we signal the condition.
+ threads::mutex &m;
+ // A reference to the parent's variable indicating
+ // whether the event has triggered. Will be set
+ // to "true" after we try to read all available
+ // keystrokes.
+ bool &b;
+ // A reference to the parent's condition variable.
+ threads::condition &c;
+
public:
- key_input_event (const key &_k)
- :k(_k)
+ get_input_event(threads::mutex &_m, bool &_b, threads::condition &_c)
+ : m(_m), b(_b), c(_c)
{
}
void dispatch()
{
- if(global_bindings.key_matches(k, "Refresh"))
- vscreen_redraw();
- else
- toplevel->dispatch_key(k);
+ // NB: use the GLOBAL getch function to avoid weirdness
+ // referencing the state of toplevel.
+ wint_t wch = 0;
+ int status;
+
+ bool done = false;
+
+ while(!done)
+ {
+ // I assume here that vscreen_init() set nodelay.
+ do
+ {
+ status = get_wch(&wch);
+ } while(status == KEY_CODE_YES && wch == KEY_RESIZE);
+
+ if(status == ERR) // No more to read.
+ {
+ threads::mutex::lock l(m);
+ b = true;
+ c.wake_all();
+ done = true;
+ }
+ else
+ {
+ key k(wch, status == KEY_CODE_YES);
+
+ if(wch == KEY_MOUSE)
+ {
+ if(toplevel.valid())
+ {
+ MEVENT ev;
+ getmouse(&ev);
+
+ toplevel->dispatch_mouse(ev.id, ev.x, ev.y, ev.z, ev.bstate);
+ }
+ }
+ else
+ {
+ if(global_bindings.key_matches(k, "Refresh"))
+ vscreen_redraw();
+ else
+ toplevel->dispatch_key(k);
+ }
+ }
+ }
}
};
- class mouse_input_event : public vscreen_event
+ class fatal_input_exception : public Exception
{
- MEVENT ev;
+ int err;
+ public:
+ fatal_input_exception(int _err)
+ : err(_err)
+ {
+ }
+ std::string errmsg() const
+ {
+ return "Unable to read from stdin: " + sstrerror(err);
+ }
+ };
+
+ class fatal_input_error : public vscreen_event
+ {
+ int err;
public:
- mouse_input_event(const MEVENT &_ev)
- :ev(_ev)
+ fatal_input_error(int _err)
+ : err(_err)
{
}
void dispatch()
{
- if(toplevel.valid())
- toplevel->dispatch_mouse(ev.id, ev.x, ev.y, ev.z, ev.bstate);
+ throw new fatal_input_exception(err);
}
};
+ threads::mutex input_event_mutex;
+ // Used to block this thread until an event to read input fires.
+ bool input_event_fired;
+ threads::condition input_event_condition;
+
static input_thread instance;
static threads::mutex instance_mutex;
static threads::thread *instancet;
+
public:
static void start()
{
threads::mutex::lock l(instance_mutex);
if(instancet == NULL)
- instancet = new threads::thread(instance);
+ instancet = new threads::thread(threads::make_bootstrap_proxy(&instance));
}
static void stop()
@@ -265,34 +346,48 @@
}
}
- void operator()() const
+ void operator()()
{
- // NB: use the GLOBAL getch function to avoid weirdness
- // referencing the state of toplevel.
- while(1)
- {
- wint_t wch = 0;
- int status;
+ threads::mutex::lock l(input_event_mutex);
+ input_event_fired = false;
- do
- {
- status = get_wch(&wch);
- } while(status == KEY_CODE_YES && wch == KEY_RESIZE);
+ // Important note: this routine only blocks indefinitely in
+ // select() and pthread_cond_wait(), assuming no bugs in
+ // vscreen_post_event(). pthread_cond_wait() is a cancellation
+ // point. select() should be but isn't, but there is a
+ // workaround below.
- key k(wch, status == KEY_CODE_YES);
-
- if(status == ERR)
- return; // ???
+ while(1)
+ {
+ // Select on stdin; when we see that input is available, spawn
+ // an input event.
+ fd_set selectfds;
+ FD_ZERO(&selectfds);
+ FD_SET(0, &selectfds);
+
+ pthread_testcancel();
+ int result = select(1, &selectfds, NULL, NULL, NULL);
+ pthread_testcancel(); // Workaround for Linux threads suckage.
+ // See pthread_cancel(3).
- if(wch == KEY_MOUSE)
+ if(result != 1)
{
- MEVENT ev;
- getmouse(&ev);
-
- vscreen_post_event(new mouse_input_event(ev));
+ if(errno != EINTR)
+ // Probably means that there was an error reading
+ // standard input. (could also be ENOMEM)
+ vscreen_post_event(new fatal_input_error(errno));
+ break;
}
else
- vscreen_post_event(new key_input_event(k));
+ {
+ vscreen_post_event(new get_input_event(input_event_mutex,
+ input_event_fired,
+ input_event_condition));
+
+ while(!input_event_fired)
+ input_event_condition.wait(l);
+ input_event_fired = false;
+ }
}
}
};
@@ -497,23 +592,6 @@
// The global instance; this is a Singleton.
static timeout_thread instance;
- // Unfortunately, technical considerations in the threading code
- // mean that the actual thread object is expected to be copyable.
- // Hence this proxy:
- class timeout_proxy
- {
- timeout_thread &real_thread;
- public:
- timeout_proxy(timeout_thread &_real_thread)
- : real_thread(_real_thread)
- {
- }
-
- void operator()() const
- {
- real_thread();
- }
- };
public:
static timeout_thread &get_instance()
{
@@ -531,7 +609,8 @@
throw SingletonViolationException();
}
- instance.running_thread.put(new threads::thread(timeout_proxy(instance)));
+ threads::thread *t = new threads::thread(threads::make_bootstrap_proxy(&instance));
+ instance.running_thread.put(t);
}
static void stop()
@@ -689,6 +768,7 @@
curses_avail=true;
cbreak();
+ rootwin.nodelay(true);
rootwin.keypad(true);
global_bindings.set("Quit", quitkey);
