1

I have a class that constantly refreshes devices physically connected to PC via USB. The monitoring method runs on a thread checking a _monitoring flag, and Start and Stop methods just set and unset that flag.

My current problem is: when the thread is running, I get the expected "busy" and "not busy" console prints, but when I call Stop method, it keeps running while(_busy) forever, because somehow the _monitoringThread seems to stop running!

I suspect it stops running because the last print is always busy, that is, the ExecuteMonitoring runs midway and then nobody knows (at least I don't).

Pause debugging and looking at StackTrace didn't help either, because it keeps in the while(_busy) statement inside Stop() method, forever.

public class DeviceMonitor
{
    bool _running;
    bool _monitoring;
    bool _busy = false;
    MonitoringMode _monitoringMode;
    Thread _monitoringThread;

    readonly object _lockObj = new object();


    // CONSTRUTOR
    public DeviceMonitor()
    {
        _monitoringThread = new Thread(new ThreadStart(ExecuteMonitoring));
        _monitoringThread.IsBackground = true;
        _running = true;
        _monitoringThread.Start();            
    }


    public void Start()
    {
        _monitoring = true;
    }

    public void Stop()
    {
        _monitoring = false;
        while (_busy)
        {                
            Thread.Sleep(5);
        }
    }


    void ExecuteMonitoring()
    {
        while (_running)
        {
            Console.WriteLine("ExecuteMonitoring()");

            if (_monitoring)
            {
                lock (_lockObj)
                {
                    _busy = true;
                }
                Console.WriteLine("busy");

                if (_monitoringMode == MonitoringMode.SearchDevices)
                {
                    SearchDevices();
                }
                else
                if (_monitoringMode == MonitoringMode.MonitorDeviceConnection)
                {
                    MonitorDeviceConnection();
                }

                lock (_lockObj)
                {
                    _busy = false;
                }
                Console.WriteLine("not busy");              
            }
            Thread.Sleep(1000);
            _busy = false;                
        }
    }

    private void SearchDevices()
    {
        var connected = ListDevices();

        if (connected.Count > 0)
        {
            Device = connected.First();
            ToggleMonitoringMode();
        }
        else
            Device = null;
    }


    void MonitorDeviceConnection()
    {
        if (Device == null)
        {
            ToggleMonitoringMode();
        }
        else
        {
            bool responding = Device.isConnected;
            Console.WriteLine("responding " + responding);
            if (!responding)
            {
                Device = null;
                ToggleMonitoringMode();
            }
        }

    }


    void ToggleMonitoringMode()
    {
        if (_monitoringMode == MonitoringMode.SearchDevices)
            _monitoringMode = MonitoringMode.MonitorDeviceConnection;
        else
        if (_monitoringMode == MonitoringMode.MonitorDeviceConnection)
            _monitoringMode = MonitoringMode.SearchDevices;
    }

    enum MonitoringMode
    {
        SearchDevices,
        MonitorDeviceConnection
    }
}
heltonbiker
  • 26,657
  • 28
  • 137
  • 252
  • `_busy` only appears 1 time in your code, and it's not even defined. Is that your REAL code? – Amit Sep 02 '15 at 14:25
  • 8
    Also, don't do that. Instead, use `ManualResetEvent`. – SLaks Sep 02 '15 at 14:25
  • @Amit thanks for pointing, it is a typo from my part, it should be `_busy` everywhere, gonna correct it. Original code not in english. – heltonbiker Sep 02 '15 at 14:28
  • Where is the Stop() method being called from ? – eran otzap Sep 02 '15 at 14:55
  • You need to replace the _busy with some Singaling event it's not just ManualResetEvent out there. Also you need a safe method of cancelling the running thread. Meanwhile I'm figuring out an optimal solution. – vendettamit Sep 02 '15 at 21:07
  • 1
    While I agree that the "busy wait" is bad, I disagree with advice to use `ManualResetEvent`. There are better, .NET-native thread synchronization mechanisms that should be used. That said, your main question seems to be why the `_busy` flag is never set to `true`; given your observation that the `_monitoringThread` actually winds up terminated, and that you never see the `"not busy"` message, it seems probable that the thread is throwing an exception before reaching the statement that clears the `_busy` flag. – Peter Duniho Sep 02 '15 at 21:52
  • Without [a good, _minimal_, _complete_ code example](https://stackoverflow.com/help/mcve) that reliably reproduces the problem, it would be impossible to provide a good, useful answer. – Peter Duniho Sep 02 '15 at 21:52

1 Answers1

1

The most likely explanation is: optimization: The compiler sees that _busy is never changed inside the Stop method and it is therefore allowed to convert this to an endless loop by replacing _busy with true. This is valid, because the _busy field is not marked as being volatile and as such the optimizer doesn't have to assume changes happening on another thread.

So, try marking _busy as volatile. Or, even better - actually A LOT BETTER - use a ManualResetEvent:

ManualResetEvent _stopMonitoring = new ManualResetEvent(false);
ManualResetEvent _monitoringStopped = new ManualResetEvent(false);
ManualResetEvent _stopRunning = new ManualResetEvent(false);

public void Stop()
{
    _stopMonitoring.Set();
    _monitoringStopped.Wait();
}

void ExecuteMonitoring()
{
    while (!_stopRunning.Wait(0))
    {
        Console.WriteLine("ExecuteMonitoring()");

        if(!_stopMonitoring.Wait(0))
        {
            _monitoringStopped.Unset();
            // ...
        }
        _monitoringStopped.Set();
        Thread.Sleep(1000);
    }
}

Code is from memory, might contain some typos.

Daniel Hilgarth
  • 171,043
  • 40
  • 335
  • 443
  • Nice, nice, nice! :) . Two thoughts, though: 1) as you already said, it looks like ["volatile is evil"](http://stackoverflow.com/a/17530556/401828); 2) I can already believe your alternative would be better, just haven't grasped yet why it would be __a lot__ better. Would you mind elaborating a bit? – heltonbiker Sep 02 '15 at 16:46
  • (by the way, declaring `volatile bool _busy` didn't solve the problem, gonna try the `ManualResetEvent` approach soon). – heltonbiker Sep 02 '15 at 17:05
  • @heltonbiker: Yes, for one, because "volatile is evil". Mainly, because it is a highly complex keyword as you can see from the answer you linked. The simplistic explanation in my answer also highlights the fact, that it is just way too easy to get wrong. But the other reason is simply that a ManualResetEvent isn't performing an active wait like your loop with the `Thread.Sleep(5)` in it. – Daniel Hilgarth Sep 02 '15 at 17:06
  • @heltonbiker: If the MRE doesn't solve the problem as well, we are missing some context here. The fact that only one answer exists shows that there is most likely no other reason *in the code you have shown*. – Daniel Hilgarth Sep 02 '15 at 17:08