[Mono-dev] Control-C handler

[Jonathan Pryor]

Thank you for reviewing this.

On Mon, 2008-01-28 at 21:10 +0100, Paolo Molaro wrote:
> On 01/28/08 Jonathan Pryor wrote:
> > > It is important (as in my initialy API sketch) that this function take
> > > the signal_info and not the dignal number, as this implementation allows
> > > only just an handler per signal in the API.
> > 
> > This has been corrected.  We now support up to 64 signal handlers within
> > a process, and the same signal can be registered multiple times; each
> > such registration gets a different index, and is thus independent.
> 
> Deregistration is handled incorrectly: if there are two handlers for the
> same signal it gets disabled at the first uninstall.

Not necessarily; it depends on the order of unregistration (if the 2nd
UnixSignal instance is destroyed before the first one is, things will
work fine).  Regardless, this is a bug that should be fixed.

> > The only other major change is the addition of a
> > UnixSignal.WaitAny(UnixSignal[]) method, which allows waiting on more
> > than one handle (since WaitHandle.WaitAny() can't currently be used with
> > WaitHandle subclasses like UnixSignal).
> 
> I suggest using params in the array argument.

I'm not sure this is a good idea, as WaitAny() is overloaded:

	static bool WaitAny(params UnixSignal[] signals);
	static bool WaitAny(UnixSignal[] signals, TimeSpan timeout);
	static bool WaitAny(UnixSignal[] signals, int timeout);

Consequently, moving from WaitAny(UnixSignal[]) to one of the other
overloads would result in inconsistent usage (adding `new
UnixSignal[]{...}', etc.).

Alternatively, we could alter the ordering so that things are always
consistent, though this would result in a different order from the
WaitHandle.WaitAny() methods:

	static bool WaitAny(params UnixSignal[] signals);
	static bool WaitAny(TimeSpan timeout, params UnixSignal[] signals);
	static bool WaitAny(int timeout, params UnixSignal[] signals);

Consequently, I'm conflicted: we either keep things as they are,
allowing consistency with WaitHandle.WaitAny() and making it easy to
change the overload used, or provide params everywhere, and differ from
WaitHandle.

> > +int
> > +Mono_Unix_UnixSignal_WaitAny (void** _signals, int count, int timeout /* milliseconds */)
> > +{
> > +	fd_set read_fds;
> > +	int mr, r;
> > +	int max_fd = 0;
> > +
> > +	signal_info** signals = (signal_info**) _signals;
> > +
> > +	mr = pthread_mutex_lock (&signals_mutex);
> > +	if (mr != 0) {
> > +		errno = mr;
> > +		return -1;
> 
> You don't unlock in the return path.

if pthread_mutex_lock() returns non-zero, it failed.  Am I supposed to
call pthread_mutex_unlock() for a failed pthread_mutex_lock() call?

The man page doesn't say, vargaz says I shouldn't and should instead
assert it's 0, which iirc io-layer also does, and I can't find my
Pthreads book, so I'm confused.

> An extensive test program that shows this stuff works before being
> committed would also help.

I only have a small one; I'll need to enhance it.

Thanks,
 - Jon