다음을 통해 공유

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

  • Anonymous
    May 20, 2005
    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.
  • Anonymous
    May 20, 2005
    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
  • Anonymous
    May 20, 2005
    SetConsoleCtrlHandler(new ControlEventHandler(Handler), true); // the ControlEventHandler delegate will get GC'ed
  • Anonymous
    May 20, 2005
    You should really synchronize access to count since it is static.
  • Anonymous
    May 20, 2005
    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.
  • Anonymous
    May 20, 2005
    The ControlEventHandler delegate should return bool.
  • Anonymous
    May 20, 2005
    Thread.Sleep(100000);

    note:
    thats 100000 miliseconds = 100 seconds, not 27 hours
  • Anonymous
    May 20, 2005
    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?
  • Anonymous
    May 20, 2005
    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 :)
  • Anonymous
    May 22, 2005
    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)
  • Anonymous
    May 22, 2005
    I think SetConsoleCtrlHandler internally creates new thread, so application will keep running even after count>10
  • Anonymous
    May 23, 2005
    The comment has been removed
  • Anonymous
    May 23, 2005
    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.
  • Anonymous
    May 23, 2005
    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.
  • Anonymous
    May 27, 2005
    In this edition, I presented a small chunk of code that attempted to hook the signal handlers, so that...