none
ManualResetEvent in long run cycle RRS feed

  • Question

  • Hi!


    In desktop application I have the following code:

    public void Start()
    {
    using (_iterationAutoResetEvent = new ManualResetEvent(false))
    {
        while (_iterationAutoResetEvent.WaitOne(Settings.IterationOffset) == false)
        {
            try
            {
                 Iterate();
            }
            catch (Exception ex)
            {
               //logging
            }
        }
    }
    }

    public void Stop()
    {
        _iterationAutoResetEvent.Set();
    }

    It works fine, but after several hours of work (8-10), the cycle stops without any logged exceptions.  The cycle iterates every 5 seconds.
    It seems something fails inside ManualResetEvent and it exits the cycle.

    Could anybody tell me what can be done to understand what is wrong.

    Maybe I am missing something and using MRE incorrectly?


    thank you


    Alexander


    • Edited by Alexander-z Thursday, September 17, 2015 9:24 AM
    Thursday, September 17, 2015 9:23 AM

Answers

  • This construct is killing me, I have to comment on it, even though it's not the direct source of your problem.

                using (_iterationAutoResetEvent = new ManualResetEvent(false))
                {
                    while (_iterationAutoResetEvent.WaitOne(RobotSettings.IterationOffset) == false)
                    {
                        try
                        {
                            _executableRobot.Iterate();

                            _lastIterationTime = DateTime.Now;
                        }
                        catch (Exception ex)
                        {
                            _logger.Log(ex.ToString(), LogMessageTypes.Error);
                        }
                    }
                }
             

    My complaint about this code is that there is a potential race condition.  If Start is executed in the worker thread, and the event that is to be set when Stop is called gets set by another thread then how does it know how far Start has gotten?  It's wrong for the thread to create its own stop event, because it can't be called externally unless it is created, and there's no way to tell if the Start proc has reached the point in the code that has created the event yet.  It's more robust to have a Synchronous Start function, that, when it returns, the thread is created and the terminate event exists.  Then have a synchronous Stop function that sets the terminate event and joins the thread.  Then have a thread procedure that executes the worker thread's main loop.  Otherwise you need yet another event, like a "ready" event that you must wait for to synchronize with the worker thread so that you know that it is running and the terminate event has been created...but then who creates that event?  there is danger is making the same design flaw a second time.  The simplest fix is that the terminate event should exist before the thread is started .. again, assuming the worker thread can be asked to stop from a different thread.

    On the other hand, if there is no chance that the stop event is set externally (for example, if it can only be called by way of the Iterate function, then you don't even need an event, because the Iterate function is called synchronously from the worker thread.  In this way, the thread can stop itself with a simple bool "done" variable.

    OK, I've gotten that off my chest.  Sorry.

    Monday, September 21, 2015 12:36 AM

All replies

  • WaitOne can throw an exception.

    This all depends on:

    1) Who else has access to _iterationAutoResetEvent (I see it isn't a local variable).  Did it change?

    2) Who else has access to Settings.IterationOffset (I see it isn't a local variable).  Did it chanage?

    3) What else is happening in your program while this is running.  (I assume this is in a thread, and there are other threads that are running, and some that may even dispose the event or adjust the IterationOffset or who knows what else, such as kill this thread.)

    4) What Iterate does.  (less likely)

    5) What your logging does.  (There's a very slim chance an error occurred and was simply not reported correctly.  I can't tell because you didn't divulge the details.  I don't suspect a problem here.  But it's not impossible that this could be the issue too, since there are no details.)


    Thursday, September 17, 2015 11:54 AM
  • I'm also worried that you Stop without Joining the thread.  And then whatever happens after Stop starts tearing down the object with the thread that was using it still running.

    It wouldn't account for a spontaneous problem (unless you are Starting and Stopping on a regular basis.)

    Thursday, September 17, 2015 11:58 AM
  • Hi Wyck!

    Thank you for your help,

    >> 1) Who else has access to _iterationAutoResetEvent (I see it isn't a local variable).  Did it change?

    Only this class, start and stop methods, no one else

    2) Who else has access to Settings.IterationOffset (I see it isn't a local variable).  Did it change?

    It is used in other places, but only for reading (is not changed)

    3) What else is happening in your program while this is running.  (I assume this is in a thread, and there are other threads that are running, and some that may even dispose the event or adjust the IterationOffset or who knows what else, such as kill this thread.)

    Right, this code is in a thread, there are other threads, but _iterationAutoResetEvent is private, so other threads cannot dispose it.

    4) What Iterate does.  (less likely)

    Actually Iterate does a lot of stuff. The program is a robot that trades on maket, Iterate does working cycle (getting new data, analyze, send/cancel orders, and so on )

    5) What your logging does.  (There's a very slim chance an error occurred and was simply not reported correctly.  I can't tell because you didn't divulge the details.  I don't suspect a problem here.  But it's not impossible that this could be the issue too, since there are no details.)

    I agree it's really slim chance, because the information is logged into database as well as into a text file (exactly for such cases, to log at least something at any cost, and it says nothing about the moment the problem occurred)

    Now I am going to wrap  
    "using (_iterationAutoResetEvent = new ManualResetEvent(false))" in additional try-catch. 

    You are right about potential problems if play with Start-Stop, 
    but the issue I am worried about happened when nobody touch the program with 99.(9)% confidence.


    Alexander

    Friday, September 18, 2015 12:35 PM
  • What path of execution does it follow when it "stops without any logged exceptions"?  Does the while statement get a false expression?  (Meaning, _iterationAutoResetEvent.WaitOne returns false?)  Or does it throw an exception?  Or does the whole process just terminate?

    If I understand correctly, then Stop is not being called in the scenario you describe that "the cycle stops without any logged exceptions".  Right?

    ...If that's the case, then you can replace the WaitOne with Sleep(5000), and remove the condition in the loop (replace it with an endless loop), and see if it still has problems?  Since you are asserting that nobody sets the event and nobody destroys the event, then you can just replace the wait-for-something-that-will-never-happen with a sleep.

    Otherwise, I'm assuming that the WaitOne has thrown an exception.  (I'm actually predicting an ObjectDisposedException).

    Does the class that contains these Start and Stop methods implement IDisposable?

    Friday, September 18, 2015 5:24 PM
  • Thank you for help!

    >>What path of execution does it follow

    It's the good question, the only think I am sure the process is not terminated.

    >>you can replace the WaitOne with Sleep(5000)

    Yes, I can do it, but Stop method is supposed to be executed. And besides that, the used approach with MRS is widely used, so it seems there is a problem in my code.

    >> Does the class that contains these Start and Stop methods implement IDisposable?

    Class doesn't implement this interface. 

    I am thinking maybe I incorrectly invoke new thread that does execution? I do in the following way:


    public partial class RobotViewControl
    {
      private readonly IRobotExecutor _robotExecutor = new RobotExecutor(LoggerKeeper.Get());

      private void BtnStart_OnClick(object sender, RoutedEventArgs e)
      {
    var robot = RobotFactory.CreateRobot(...)

    Task.Factory.StartNew(() => _robotExecutor.Start(robot), TaskCreationOptions.LongRunning | TaskCreationOptions.AttachedToParent).LogExceptions(_logger);
      }
    }


     public class RobotExecutor
      {        
            private IExecutableRobot _executableRobot;
            private readonly ILogger _logger;
            private bool _isExecutionStarted;
            private ManualResetEvent _iterationAutoResetEvent;
            private DateTime _lastIterationTime = DateTime.MinValue;

            public RobotExecutor(ILogger logger)
            {
                _logger = logger;
            }

            public DateTime GetLastIterationTime()
            {
                return _lastIterationTime;
            }

            public bool IsExecutionStarted
            {
                get { return _isExecutionStarted; }
            }

            public void Start(IExecutableRobot robot)
            {
                if (IsExecutionStarted)
                {
                    _logger.Log("Attempt to start execution when IsExecutionStarted flag is true!", LogMessageTypes.Warning);
                    return;
                }

                _executableRobot = robot;

                _isExecutionStarted = true;

                using (_iterationAutoResetEvent = new ManualResetEvent(false))
                {
                    while (_iterationAutoResetEvent.WaitOne(RobotSettings.IterationOffset) == false)
                    {
                        try
                        {
                            _executableRobot.Iterate();

                            _lastIterationTime = DateTime.Now;
                        }
                        catch (Exception ex)
                        {
                            _logger.Log(ex.ToString(), LogMessageTypes.Error);
                        }
                    }
                }
            }

            public void Stop()
            {
                if (IsExecutionStarted)
                {
                    _iterationAutoResetEvent.Set();

                    _isExecutionStarted = false;                
                }
            }        
        }

        
        public static class TaskExtentions
        {
            public static void LogExceptions(this Task task, ILogger logger)
            {
                task.ContinueWith(t =>
                {
                    var aggException = t.Exception.Flatten();
                    foreach (var exception in aggException.InnerExceptions)
                        logger.Log(exception.ToString(), LogMessageTypes.Error);
                },
                    TaskContinuationOptions.OnlyOnFaulted);
            }
       }



    Alexander

    Sunday, September 20, 2015 8:02 PM
  • This construct is killing me, I have to comment on it, even though it's not the direct source of your problem.

                using (_iterationAutoResetEvent = new ManualResetEvent(false))
                {
                    while (_iterationAutoResetEvent.WaitOne(RobotSettings.IterationOffset) == false)
                    {
                        try
                        {
                            _executableRobot.Iterate();

                            _lastIterationTime = DateTime.Now;
                        }
                        catch (Exception ex)
                        {
                            _logger.Log(ex.ToString(), LogMessageTypes.Error);
                        }
                    }
                }
             

    My complaint about this code is that there is a potential race condition.  If Start is executed in the worker thread, and the event that is to be set when Stop is called gets set by another thread then how does it know how far Start has gotten?  It's wrong for the thread to create its own stop event, because it can't be called externally unless it is created, and there's no way to tell if the Start proc has reached the point in the code that has created the event yet.  It's more robust to have a Synchronous Start function, that, when it returns, the thread is created and the terminate event exists.  Then have a synchronous Stop function that sets the terminate event and joins the thread.  Then have a thread procedure that executes the worker thread's main loop.  Otherwise you need yet another event, like a "ready" event that you must wait for to synchronize with the worker thread so that you know that it is running and the terminate event has been created...but then who creates that event?  there is danger is making the same design flaw a second time.  The simplest fix is that the terminate event should exist before the thread is started .. again, assuming the worker thread can be asked to stop from a different thread.

    On the other hand, if there is no chance that the stop event is set externally (for example, if it can only be called by way of the Iterate function, then you don't even need an event, because the Iterate function is called synchronously from the worker thread.  In this way, the thread can stop itself with a simple bool "done" variable.

    OK, I've gotten that off my chest.  Sorry.

    Monday, September 21, 2015 12:36 AM