[Mono-dev] [PATCH] mono-service bugs - please review

[Avery Pennarun]

On Mon, Feb 04, 2008 at 02:12:48PM -0500, Jonathan Pryor wrote:

> On Mon, 2008-02-04 at 12:14 -0500, Avery Pennarun wrote:
> > 1. mono-service runner doesn't catch SIGINT (it should clean up like
> > SIGTERM, and this is especially important when using the --debug
> > option).
> 
> Not as easy as you'd think; check the archives with the "Control-C
> handler" subject.  To reliably catch SIGINT/SIGTERM/etc., you'll need to
> use UnixSignal, which was just added to svn-HEAD.  If I'm lucky, I'll
> get the tests written to permit it to go into 1.9 soon.

I read that thread.  Luckily or unluckily, mono-service already catches
various signals; SIGINT should be treated identically to SIGTERM, so making
it work "better" should be a one-liner.

Note that it currently is implemented by polling every 500ms, which is lame
on multiple levels (you don't want daemons waking up every 500ms on your
laptop for no reason, as it wastes power).  I eagerly await your signal
patch.

> >   Or maybe it should always be true except when
> > mono-service explicitly makes it false somehow?
> 
> This makes more sense.

Implemented.

> You can [DllImport("__Internal")] to obtain functions within the mono
> executable, or (better) you can add an internal call to Mono that
> mono-service would make use of.  Environment.UserInteractive could then
> use a different internal call to obtain the value of some variable,
> permitting communication (via internal calls) between mono-service and
> System.Environment.  This is likely the best solution.

I'm afraid I had a hard time deciding between the various methods here.  It
didn't seem to me that an InternalCall was a very good choice, since we're
just talking about a single bool and there's no reason that bool should be
managed in native code.

I did something else instead based on what was suggested elsewhere in this
thread (private Environment.SetUserInteractive method that mono-service
invokes via reflection).  I'm a little fuzzy on exactly what goes where for
mono ABI compatibility, so please let me know if I've done it wrong.

See the attached patch.  Changes:

- mono-service makes Environment.UserInteractive false, otherwise defaults
  to true.  (NOTE: this is the opposite of the old default!!)

- mono-service can overwrite its lockfile if the lockfile isn't
  lockf'ed.  This seems to be what was originally intended, but didn't
  quite get implemented correctly.
  
- SIGINT is now trapped and treated the same as SIGTERM, which is nice when
  using the --debug option.

Any chance this can go into the mono-1-9 branch?

Have fun,

Avery
-------------- next part --------------
diff --git a/mcs/class/corlib/System/Environment.cs b/mcs/class/corlib/System/Environment.cs
index 9e09405..ff6ca28 100644
--- a/mcs/class/corlib/System/Environment.cs
+++ b/mcs/class/corlib/System/Environment.cs
@@ -232,14 +232,21 @@ namespace System {
 				return MachineName;
 			}
 		}
-
+		
+		private static bool IsInteractive = true;
+		
+		private static void SetUserInteractive(bool Value)
+		{
+			IsInteractive = Value;
+		}
+		
 		/// <summary>
-		/// Gets a flag indicating whether the process is in interactive mode
+		/// Gets a flag indicating whether the process is in interactive mode.
+		/// Running under mono-service sets this to false.
 		/// </summary>
-		[MonoTODO ("Currently always returns false, regardless of interactive state")]
 		public static bool UserInteractive {
 			get {
-				return false;
+				return IsInteractive;
 			}
 		}
 
diff --git a/mcs/tools/mono-service/mono-service.cs b/mcs/tools/mono-service/mono-service.cs
index 41c4931..54379e4 100644
--- a/mcs/tools/mono-service/mono-service.cs
+++ b/mcs/tools/mono-service/mono-service.cs
@@ -100,10 +100,13 @@ class MonoServiceRunner : MarshalByRefObject
 		if (lockfile == null)
 			lockfile = String.Format ("/tmp/{0}.lock", Path.GetFileName (assembly));
 
-		int lfp = Syscall.open (lockfile, OpenFlags.O_RDWR|OpenFlags.O_CREAT|OpenFlags.O_EXCL, 
+		int lfp = Syscall.open (lockfile, OpenFlags.O_RDWR, 
 			FilePermissions.S_IRUSR|FilePermissions.S_IWUSR|FilePermissions.S_IRGRP);
+		if (lfp<0)
+			lfp = Syscall.open (lockfile, OpenFlags.O_RDWR|OpenFlags.O_CREAT|OpenFlags.O_EXCL, 
+				FilePermissions.S_IRUSR|FilePermissions.S_IWUSR|FilePermissions.S_IRGRP);
 
-		if (lfp<0)  {
+		if (lfp<0) {
 		        // Provide some useful info
 			if (File.Exists (lockfile))
 				error (logname, String.Format ("Lock file already exists: {0}", lockfile));
@@ -119,6 +122,7 @@ class MonoServiceRunner : MarshalByRefObject
 		
 		try {
 			// Write pid to lock file
+			Syscall.ftruncate (lfp, 0);
 			string pid = Syscall.getpid ().ToString () + Environment.NewLine;
 			IntPtr buf = Marshal.StringToCoTaskMemAnsi (pid);
 			Syscall.write (lfp, buf, (ulong)pid.Length);
@@ -169,9 +173,18 @@ class MonoServiceRunner : MarshalByRefObject
 			
 			// Hook up 
 			Stdlib.signal (Signum.SIGTERM, new SignalHandler (my_handler));
+			Stdlib.signal (Signum.SIGINT,  new SignalHandler (my_handler));
 			Stdlib.signal (Signum.SIGUSR1, new SignalHandler (my_handler));
 			Stdlib.signal (Signum.SIGUSR2, new SignalHandler (my_handler));
 	
+			// Tell the application we're not interactive
+			// (ie. no GUI)
+			Type.GetType ("System.Environment")
+				.GetMethod ("SetUserInteractive",
+					    System.Reflection.BindingFlags.Static
+					  | System.Reflection.BindingFlags.NonPublic)
+				.Invoke (null, new object[] { false });
+			
 			// Load service assembly
 			Assembly a = null;
 			
@@ -261,6 +274,7 @@ class MonoServiceRunner : MarshalByRefObject
 					
 					switch (v){
 					case Signum.SIGTERM:
+					case Signum.SIGINT:
 						if (service.CanStop) {
 							info (logname, "Stopping service {0}", service.ServiceName);
 							call (service, "OnStop", null);