[Mono-dev] Control-C handler

[Paolo Molaro]

On 01/10/08 Jonathan Pryor wrote:
> Attached is an updated patch set which supports both the existing/new
> Stdlib.signal() semantics (as the previous patch set did) in which the
> signal handler is invoked on an internal helper thread, and adds a new
> Mono.Unix.UnixSignal type to permit polling and/or blocking as lupus
> originally suggested.

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.

> The one thing I don't like (but can't think of a workaround) is the
> interaction between UnixSignal and Stdlib.signal(): as currently
> implemented, UnixSignal and Stdlib.signal() can't handle the same signal
> simultaneously -- if UnixSignal registered to handle e.g. SIGINT, then
> Stdlib.signal() will return SIG_ERR, with errno set to EEXIST.
> 
> However, the alternate IS possible: UnixSignal can handle e.g. SIGINT if
> Stdlib.signal() was previously called to handle it (and will
> consequently replace the original Stdlib.signal() handler).  The
> "original" Stdlib.signal() handler will be replaced when the UnixSignal
> instance is destroyed.

I think there are two choices for Stdlib.signal(): either leave it alone
or make it do nothing/throw an exception (this can happen after some
time it has been obsoleted).

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

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

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

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

> +			if (signal_dispatcher == null) {
> +				object c = new Thread (SignalDispatcher);
> +				Thread.MemoryBarrier ();
> +				while (Interlocked.CompareExchange (ref signal_dispatcher, c, null) == null) {
> +					int writing;
> +					if (Syscall.pipe (out signal_read_fd, out writing) < 0)
> +						throw UnixMarshal.CreateExceptionForLastError ();
> +					set_signal_write_fd (writing);
> +					Thread _c = (Thread) c;
> +					_c.IsBackground = true;
> +					_c.Name = "Mono.Unix.Native Signal Dispatcher";
> +					_c.Start ();
> +				}
> +				Thread.MemoryBarrier ();
> +			}
> +		}
> +
> +		private static unsafe void SignalDispatcher ()
> +		{
> +			byte c;
> +			while (Syscall.read (signal_read_fd, &c, 1) >= 1) {
> +				SignalHandler h = registered_signals [c];
> +				if (h != null)
> +					h (c);
> +			}
> +		}

There is no protection here about a callback throwing an exception, so
the thread dies and everything falls apart. It's a bad idea anyway to
try to support the broken interface and provide different semantics at
the same time.

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

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

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

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

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better