What’s wrong with this code #7?


This time, I’ll present a runnable program. The ConsoleCtrl class is designed to intercept CTRL-C and, for testing purposes, will exit after it gets 10 of them. Or after a little over 27 hours.


What’s wrong with this code?


using System;
using System.Threading;
using System.Runtime.InteropServices;


public class ConsoleCtrl
{


 public enum ConsoleEvent
 {
  CTRL_C = 0,  // From wincom.h
  CTRL_BREAK = 1,
  CTRL_CLOSE = 2,
  CTRL_LOGOFF = 5,
  CTRL_SHUTDOWN = 6
 }


 public delegate void ControlEventHandler(ConsoleEvent consoleEvent);
 public event ControlEventHandler ControlEvent;


 public ConsoleCtrl()
 {
  SetConsoleCtrlHandler(new ControlEventHandler(Handler), true);
 }


 private void Handler(ConsoleEvent consoleEvent)
 {
  if (ControlEvent != null)
   ControlEvent(consoleEvent);
 }


 [DllImport(“kernel32.dll”)]
 static extern bool SetConsoleCtrlHandler(ControlEventHandler e, bool add);
}



public class Program
{
 static int count = 0;


 public static void Main()
 {
  ConsoleCtrl consoleCtrl = new ConsoleCtrl();


  consoleCtrl.ControlEvent += new ConsoleCtrl.ControlEventHandler(Logger);


  Thread.Sleep(100000);
 }


 private static void Logger(ConsoleCtrl.ConsoleEvent consoleEvent)
 {
  Console.WriteLine(consoleEvent);
  count++;


  if (count == 10)
  {
   Environment.Exit(0);
  }
 }


}


 

Comments (15)

  1. Chris Hind says:

    If you want it to exit after 10 CTRL_C’s, and only after 10 CTRL_C’s, then you’ll be upset when it exits after 10 CTRL_BREAK’s.

  2. marek says:

    Hello,

    I think that the problem is in the definition of the event handler. WinAPI SetConsoleCtrlHandler requires the prototype of BOOL Handler(DWORD). So your Logger() method should return bool – this applies for all the definitions of the delegates and events. In this case it is important because the return value specifies whether the system will call next handlers or not. If we don’t return something, some random value from stack or a register will be used as a return parameter.

    The second think I can think of is the compatibility between enum underlaying type and the DWORD. But it seems OK here (32 bit)

    I would write the code like this:

    private static bool Logger(ConsoleCtrl.ConsoleEvent consoleEvent)

    {… return true;/* i want only my handler*/}

    + approprite corrections to events and delegates.

    Reagrds

    Marek

  3. Ali A says:

    SetConsoleCtrlHandler(new ControlEventHandler(Handler), true); // the ControlEventHandler delegate will get GC’ed

  4. Bob says:

    You should really synchronize access to count since it is static.

  5. Mike Treffry says:

    The callback function you pass in:

    SetConsoleCtrlHandler(new ControlEventHandler(Handler), true);

    is only pinned for the length of the call to SetConsoleCtrlHandler() and hence it will be eligible for garbage collection after the call. So start pressing Ctrl-C pretty quick.

  6. Andrew says:

    The ControlEventHandler delegate should return bool.

  7. Versus says:

    Thread.Sleep(100000);

    note:

    thats 100000 miliseconds = 100 seconds, not 27 hours

  8. Sheeshers says:

    Where are you checking to make sure you only respond to a CTRL-C event? Or maybe you’re looking for something more than just such trivial stuff?

  9. Matt Garven says:

    Not only may the ControlEventHandler get GC’d, but the ConsoleCtrl instance itself will also be eligible for garbage collection (an optimisation in release mode only – during debug it won’t be eligible until the end of the method to allow debuggers to inspect the value). I think 🙂

  10. count++;

    if (count == 10)

    {

    Environment.Exit(0);

    }

    is not thread safe, so it is possible that even though count > 10 and progam is still running. 😉

    Use Interlocked.Increment(ref int)

  11. I think SetConsoleCtrlHandler internally creates new thread, so application will keep running even after count>10

  12. Felipe Garcia says:

    I think that "the code" will work fine. The problem I see is related to the Thread.Sleep that will hold the execution of the program for "a while" (100000 / 60) / 60 is like ~27.8 hours.

  13. mgd says:

    I tend to agree with the idea that delegate may get GC’d; since it’s passed into unmanaged code the GC will have no way of telling that it’s still required.

    But since you can’t count of when GC will or won’t happen… I would be surprised if it was not usually be the case that it was still around.

  14. Phil Hochstetler says:

    The most obvious thing I see wrong is assuming the count goes from 9 to 10. The count could be incremented twice before the test so it would appear to go from 9 to 11. Either a lock() should be added around the increment / test, or a interlocked call used or just test for >=. All would fix the problem.

  15. In this edition, I presented a small chunk of code that attempted to hook the signal handlers, so that…