[Mono-dev] Control-C handler

[Jonathan Pryor]

On Fri, 2008-01-11 at 19:56 +0100, Paolo Molaro wrote:
> On 01/10/08 Jonathan Pryor wrote:
> > Attached is an updated patch set which supports both the existing/new
> > Stdlib.signal() semantics...
> 
> I think signal() should just be obsoleted, starting a thread yourself
> doesn't provide the same semantics and it is doesn't allow the user much
> control.

OK.  But...

> > Of course, this could be used as an argument to dump Stdlib.signal()
> > support entirely, but it needs to stick around to permit setting SIG_IGN
> > on a given signal.  I suppose Stdlib.signal() could be restricted to
> > just accepting SIG_IGN, SIG_ERR, and SIG_DFL and error out if any other
> 
> Just provide a sane interface to ignore a signal instead of this kludge.

What exactly would be a "sane interface to ignore a signal" aside from
e.g. Stdlib.signal(Signum.SIGINT, Stdlib.SIG_IGN) (and/or setting the
default or error handler for the specified signal)?  The current
Stdlib.signal() interface seems quite sane for that (to me), and results
in a fairly sensible implementation:

	public static SignalHandler signal (Signum sig, SignalHandler h)
	{
	  if (h != SIG_IGN && h != SIG_DFL && h != SIG_ERR)
	    throw new UnixIOException (...);
	  return TranslateHandler (sys_signal (sig, h));
	}

> > delegate instance is passed, so that people use UnixSignal for all
> > significant signal usage, but I don't like this either as currently each
> > UnixSignal.WaitOne() call requires a separate pipe, while
> 
> This is an implementation detail of your version, it could be done
> differently.

Yes, but the question is at what cost?  With one pipe/UnixSignal
instance, WaitOne() will ONLY be woken up once the pipe is ready.  With
a all UnixSignal instances sharing a single pipe, ALL instances will
wake up once the pipe is ready, and the only way of knowing which
signal(s) was written was to read(2) the pipe, which only one UnixSignal
instance can do...

I'm sure there's a better way to do it, but I can't think of it yet.
Suggestions welcome. :-)

> > Index: Mono.Unix.Native/Stdlib.cs
> > ===================================================================
> > --- Mono.Unix.Native/Stdlib.cs	(revision 92060)
> > +++ Mono.Unix.Native/Stdlib.cs	(working copy)
> > @@ -31,6 +31,9 @@
> >  using System.IO;
> >  using System.Runtime.InteropServices;
> >  using System.Text;
> > +using System.Threading;
> > +
> > +using Mono.Unix;
> >  using Mono.Unix.Native;
> >  
> >  namespace Mono.Unix.Native {
> > @@ -359,7 +362,7 @@
> >  	//        This is the case for using NativeConvert, which will throw an
> >  	//        exception if an invalid/unsupported value is used.
> >  	//
> > -	public class Stdlib
> > +	public unsafe class Stdlib
> 
> It's a bad idea to make the whole class unsafe.

Indeed, especially when only one method needs it.  For some reason I
thought if any method needed unsafe, the class did as well -- insane.

> > @@ -494,6 +495,45 @@
> >  #endif
> >  		}
> >  
> > +		[DllImport (MPH, CallingConvention=CallingConvention.Cdecl,
> > +				SetLastError=true, EntryPoint="_mph_set_signal_write_fd")]
> > +		private static extern void set_signal_write_fd (int signum);
> > +
> > +		static object signal_dispatcher;
> > +		static int    signal_read_fd;
> > +
> > +		private static void InitSignalSupport ()
> > +		{
> > +			if (Path.DirectorySeparatorChar == '\\')
> > +				return;
> 
> This looks ugly.

This is no longer necessary if we only support UnixSignal and not the
separate-thread Stdlib.signal() behavior.

> > --- Mono.Unix/UnixSignal.cs	(revision 0)
> > +++ Mono.Unix/UnixSignal.cs	(revision 0)
> [...]
> > +namespace Mono.Unix {
> > +	public class UnixSignal : WaitHandle {
> > +		private Signum signum;
> > +		private int _signum;
> > +		private IntPtr signal_info;
> > +
> > +		public UnixSignal (Signum signum)
> > +		{
> > +			this.signum = signum;
> > +			this._signum = NativeConvert.FromSignum (signum);
> > +			lock (Stdlib.registered_signals) {
> > +				this.signal_info = install (_signum);
> > +			}
> > +			if (this.signal_info == IntPtr.Zero) {
> > +				throw new ArgumentException ("Unable to handle signal", "signum");
> > +			}
> > +		}
> > +
> > +		public Signum Signum {
> > +			get { return signum; }
> > +		}
> > +
> > +		[DllImport (Stdlib.MPH, CallingConvention=CallingConvention.Cdecl,
> > +				EntryPoint="Mono_Unix_UnixSignal_install")]
> > +		private static extern IntPtr install (int signum);
> > +
> > +		[DllImport (Stdlib.MPH, CallingConvention=CallingConvention.Cdecl,
> > +				EntryPoint="Mono_Unix_UnixSignal_uninstall")]
> > +		private static extern void uninstall (int signum);
> 
> My proposed interface allows registering multiple handlers per signals
> and this code can deal with just one.

That's another one of those "I'm not sure how else to implement that"
issues, largely tied to the previous one regarding pipes.

> > +		public unsafe override bool WaitOne (int millisecondsTimeout, bool exitContext)
> > +		{
> > +			AssertValid ();
> > +			if (exitContext)
> > +				throw new InvalidOperationException ("exitContext is not supported");
> > +			if (millisecondsTimeout < -1)
> > +				throw new ArgumentOutOfRangeException ("millisecondsTimeout");
> > +			int read, write;
> > +			bool close = true;
> > +			read = write = -1;
> > +			try {
> > +				if (Syscall.pipe (out read, out write) < 0) {
> > +					close = false;
> > +					throw UnixMarshal.CreateExceptionForLastError ();
> > +				}
> > +				Pollfd[] fds  = new Pollfd[1];
> > +				fds[0].fd     = read;
> > +				fds[0].events = PollEvents.POLLIN;
> > +				if (IsSet)
> > +					return true;
> > +				Info->write_fd = write;
> > +				int r = Syscall.poll (fds, 1, millisecondsTimeout);
> 
> Unless Syscall.poll() actually falls back to select() it shouldn't be
> used as for example the POS OS X doesn't implement it.

Which is odd, as OS X is documented as supporting it:
http://developer.apple.com/documentation/Darwin/Reference/ManPages/man2/poll.2.html

> > +				if (r == -1 && Stdlib.GetLastError () != Errno.EINTR) {
> > +					return false;
> > +				}
> > +				else
> > +					r = Syscall.poll (fds, 1, 0);
> > +				if (r > 0) {
> > +					byte c = 0;
> > +					Syscall.read (read, &c, 1);
> 
> No error checking. This stuff should be implemented in C anyway.

I'm not sure what error checking this needs, as poll(2) says it's
readable in order to reach it; for that matter, I'm not sure what to do
on error here either -- retry the poll/read combo?

> > --- signal.c	(revision 92060)
> > +++ signal.c	(working copy)
> > +void*
> > +Mono_Unix_UnixSignal_install (int sig)
> > +{
> > +	signal_info* n = realloc (registered_signals, 
> > +			sizeof(signal_info)*(registered_signals_count+1));
> > +	if (n) {
> > +		signal_info* r;
> > +		registered_signals = n;
> > +		r = &registered_signals [registered_signals_count++];
> > +
> > +		r->signum   = sig;
> > +		r->count    = 0;
> > +		r->write_fd = 0;
> > +
> > +		if ((r->handler = signal (sig, default_handler)) == SIG_ERR) {
> > +			Mono_Posix_Signal_uninstall (sig);
> > +			return NULL;
> > +		}
> > +		return r;
> > +	}
> > +	return NULL;
> > +}
> 
> The handling of the registered_signals array is not thread safe and not
> signal safe.

I don't see why this would need to be signal safe -- it should never be
called from a signal handler -- and it is already thread safe, as
managed code performs the locking: the UnixSignal constructor and
UnixSignal.Dispose() both acquire the Stdlib.registered_signals lock
before calling into this code, so only one thread can execute it at a
time anyway.

 - Jon