Sign in to follow this  

Weird stuff happening when stepping through a threaded code

This topic is 1103 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Update 1:-

Problem solved I was calling the Thread twice. I'm so stupid. happy.png

 

Update 2:- as it turns out my code is not thread safe. working to fix it and will post new code soon.

 

Hello all,

 

I have very little experience in Multi-threaded programming, but from what I understand is you can create a thread to solve a problem and when its done it spits out the answer for your problem.

 

So that's what I did. I have a thread that starts and never dies until the software exits. in this thread I try to solve pathfinding problems. I have a Job class and a result class. In my main class I have a list of both the job class and the result class. So basically what I do is, I add new jobs to the list and the UpdateThread takes these jobs and solve them. When its done solving them, it spits out the answer in the results list and then deletes the old job from the job list.

 

My problem is sometimes I get random null errors inside my Astar class which is very weird. However there is another weird problem which is better demonstrated in the video below.

 

So when I try to step into my update code which is the threaded code, I seem to step in it twice. meaning I have to press F11 twice to get to the next line. which is really strange. Can someone please explain why is this happening?

 

http://www.youtube.com/watch?v=-iAuVNmehDw

 

 

 

and Here is the code:-

namespace Engine.AI.Pathfinding
{
    class AstarJob
    {
        internal Astar aStar;
        internal int JobNumber;

        internal AstarJob(Node StartingNode, Node TargetNode, Map map, bool DisableDiagonalPathfinding, int JobNumber)
        {
            this.JobNumber = JobNumber;
            aStar = new Astar(StartingNode, TargetNode, map, DisableDiagonalPathfinding);
        }
    }

    class AstarResult
    {
        internal List<Vector2> Path;
        internal int JobNumber;

        internal AstarResult(List<Vector2> Path, int JobNumber)
        {
            this.Path = Path;
            this.JobNumber = JobNumber;
        }
    }

    public class AstarManager
    {
        static List<AstarJob> AstarJobsList;
        static List<AstarResult> AstarResultList;

        Thread UpdateThread;

        public AstarManager()
        {
            AstarJobsList = new List<AstarJob>();
            AstarResultList = new List<AstarResult>();

            UpdateThread = new Thread(new ThreadStart(Update));
            UpdateThread.IsBackground = true;
            UpdateThread.Priority = ThreadPriority.Highest;
            UpdateThread.Start();
        }

        public static void AddNewPath(Node StartingNode, Node TargetNode, Map map, bool DisableDiagonalPathfinding, int JobNumber)
        {
            for (int i = 0; i < AstarJobsList.Count; i++)
                if (AstarJobsList[i].JobNumber == JobNumber)
                    throw new ArgumentException("A job already exist with this JobNumber: " + JobNumber, "JobNumber");

            AstarJobsList.Add(new AstarJob(StartingNode, TargetNode, map, DisableDiagonalPathfinding, JobNumber));
        }

        public static List<Vector2> GetPath(int JobNumber)
        {
            for (int i = 0; i < AstarResultList.Count; i++)
                if (AstarResultList[i].JobNumber == JobNumber)
                    return AstarResultList[i].Path;
                    
            return new List<Vector2>();
        }

        public static bool IsJobDone(int JobNumber)
        {
            for (int i = 0; i < AstarResultList.Count; i++)
                if (AstarResultList[i].JobNumber == JobNumber)
                    return true;

            return false;
        }

        public static void RemoveJobResultAt(int JobNumber)
        {
            for (int i = 0; i < AstarResultList.Count; i++)
                if (AstarResultList[i].JobNumber == JobNumber)
                    AstarResultList.RemoveAt(i);
        }

        void Update()
        {
            while (true)
            {
                for (int i = 0; i < AstarJobsList.Count; i++)
                {
                    AstarJobsList[i].aStar.FindPath();
                    AstarResultList.Add(new AstarResult(AstarJobsList[i].aStar.GetFinalPath(), AstarJobsList[i].JobNumber));
                    AstarJobsList.RemoveAt(i);
                }
            }
        }
    }
}

Edited by FantasyVII

Share this post


Link to post
Share on other sites

first of all, your code is not thread safe, calling AddNewPath, GetPath, IsJobDone or RemoveJobresultAt while your thread is running is likely to end badly.

 

Multiple instances of AstarManager will also share data but run their own threads (again, without synchronization this will result in wierd bugs, if you are entering the update method multiple times then you are most likely creating multiple instances of AstarManager)

 

 

Edit: You havn't solved anything, Calling the thread twice only made the race-conditions more likely to occur, your code still isn't thread safe and thus can explode pretty much at any time. (Whenever you call a static method on AstarManager there is a small chance that it will blow up in your face as they modify data that the update loop assumes remains unchanged during an iteration)

Edited by SimonForsman

Share this post


Link to post
Share on other sites

first of all, your code is not thread safe, calling AddNewPath, GetPath, IsJobDone or RemoveJobresultAt while your thread is running is likely to end badly.

 

Multiple instances of AstarManager will also share data but run their own threads (again, without synchronization this will result in wierd bugs, if you are entering the update method multiple times then you are most likely creating multiple instances of AstarManager)

 

 

Edit: You havn't solved anything, Calling the thread twice only made the race-conditions more likely to occur, your code still isn't thread safe and thus can explode pretty much at any time. (Whenever you call a static method on AstarManager there is a small chance that it will blow up in your face as they modify data that the update loop assumes remains unchanged during an iteration)

 

 

My intention is to call the AstarManager once and never more than once. and then through the life duration of my software I would add jobs and remove them when they are done.

 

My understanding is that my code is thread safe unless I call AstarManager more than once. which I will not do.

 

just out of curiosity. what do I have to do to make my code thread safe? what am I doing wrong?

Edited by FantasyVII

Share this post


Link to post
Share on other sites

 

first of all, your code is not thread safe, calling AddNewPath, GetPath, IsJobDone or RemoveJobresultAt while your thread is running is likely to end badly.

 

Multiple instances of AstarManager will also share data but run their own threads (again, without synchronization this will result in wierd bugs, if you are entering the update method multiple times then you are most likely creating multiple instances of AstarManager)

 

 

Edit: You havn't solved anything, Calling the thread twice only made the race-conditions more likely to occur, your code still isn't thread safe and thus can explode pretty much at any time. (Whenever you call a static method on AstarManager there is a small chance that it will blow up in your face as they modify data that the update loop assumes remains unchanged during an iteration)

 

 

My intention is to call the AstarManager once and never more than once. and then through the life duration of my software I would add jobs and remove them when they are done.

 

My understanding is that my code is thread safe unless I call AstarManager more than once. which I will not do.

 

just out of curiosity. what do I have to do to make my code thread safe? what am I doing wrong?

 

 

To make your code thread safe you should remove all statics(they make things far more complicated and don't really serve any useful purpose in this case) and ensure that all data manipulation is either synchronized or strictly controlled.

 

consider this method:

public static void AddNewPath(Node StartingNode, Node TargetNode, Map map, bool DisableDiagonalPathfinding, int JobNumber)
{
    for (int i = 0; i < AstarJobsList.Count; i++) //lets say that AstarJobsList.Count is 8 here
       //your main thread is happily running this loop, it is on its last iteration and i is 7 (still perfectly ok).
        // your update thread completes a job and removes it from the AstarJobsList (this doesn't look good)
        if (AstarJobsList[i].JobNumber == JobNumber) // AstarJobsList[i] no longer exists, booom !
            throw new ArgumentException("A job already exist with this JobNumber: " + JobNumber, "JobNumber");
        AstarJobsList.Add(new AstarJob(StartingNode, TargetNode, map, DisableDiagonalPathfinding, JobNumber));
}
Edited by SimonForsman

Share this post


Link to post
Share on other sites

 

 


To make your code thread safe you should remove all statics(they make things far more complicated and don't really serve any useful purpose in this case) and ensure that all data manipulation is either synchronized or strictly controlled.

 

consider this method:

public static void AddNewPath(Node StartingNode, Node TargetNode, Map map, bool DisableDiagonalPathfinding, int JobNumber)
{
    for (int i = 0; i < AstarJobsList.Count; i++) //lets say that AstarJobsList.Count is 8 here
       //your main thread is happily running this loop, it is on its last iteration and i is 7 (still perfectly ok).
        // your update thread completes a job and removes it from the AstarJobsList (this doesn't look good)
        if (AstarJobsList[i].JobNumber == JobNumber) // AstarJobsList[i] no longer exists, booom !
            throw new ArgumentException("A job already exist with this JobNumber: " + JobNumber, "JobNumber");
        AstarJobsList.Add(new AstarJob(StartingNode, TargetNode, map, DisableDiagonalPathfinding, JobNumber));
}

 

 

ooh, I see. When I tested this code I never added more than one job. I see now.

 

ok, let me rethink my code structure and try to solve this issue and will post the new code soon.

 

thank you happy.png

Edited by FantasyVII

Share this post


Link to post
Share on other sites

Sorry about not deleting it. It helps allows other people to learn.

 

That is what gives this site -- and so many others -- their value. Imagine if the major programming sites like StackOverflow or StackExchange deleted questions once they were answered.  Instead of deleting them, sites provide resources with millions of little snippets of code for common mistakes.

 

no, its fine. I thought I did a silly mistake and I fixed it, but as it turns out my whole code structure is bad ^_^

 

I should update the thread lol.

Share this post


Link to post
Share on other sites

well after hours of research I think the answer is using the lock keyword. I have no idea how to do it. I have no idea what it does, but I will hopefully find out.

 

Edit:-

 

Is this correct? will adding the lock keyword here solve my problem? I'm not too sure.

void Update()
        {
            while (true)
            {
                for (int i = 0; i < AstarJobsList.Count; i++)
                {
                    lock (this)
                    {
                        AstarJobsList[i].aStar.FindPath();
                        AstarResultList.Add(new AstarResult(AstarJobsList[i].aStar.GetFinalPath(), AstarJobsList[i].JobNumber));
                        AstarJobsList.RemoveAt(i);
                    }
                }
            }
        }
Edited by FantasyVII

Share this post


Link to post
Share on other sites
No. AstarJobsList.Count is also unprotected now. Your A* thread will also consume 100% CPU time when there's no work for it to do as it's not politely waiting for work to be added to the queue and instead polling it as fast as it possibly can.

Locks are also a terrible way to handle threading in any case. Locks are weapons of mass destruction where all you usually need is a polite shove in the right direction.

Since you're using C# (I think), just use a ConcurrentQueue for the requests and another queue or Futures for the results instead of trying to invent your own broken and poorly-performing job queue.

.NET bundles a number of other higher-level threading primitives to help make all of this easier, safer, and faster. Edited by SeanMiddleditch

Share this post


Link to post
Share on other sites
Generally if you have access to the more recent versions of .Net, you should be using stuff from the Task Parallel Library ( http://msdn.microsoft.com/en-us/library/dd460717(v=vs.110).aspx )


If you're using older versions of .Net, what you're doing sounds like it would work with ThreadPool.QueueUserWorkItem ( http://msdn.microsoft.com/en-us/library/4yd16hza(v=vs.110).aspx ) pretty easily.

Share this post


Link to post
Share on other sites

Ok, finally after hours of research and coding, I think I have a thread safe code. Maybe?

 

Here, Am I doing anything stupid here?

namespace Engine.AI.Pathfinding
{
    public static class AstarManager
    {
        static Thread UpdateThread = new Thread(new ThreadStart(Update));

        static ConcurrentQueue<Astar> AstarQueue = new ConcurrentQueue<Astar>();
        public static Queue<Astar> AstarResults = new Queue<Astar>();

        public static void Initialize()
        {
            UpdateThread.IsBackground = true;
            UpdateThread.Priority = ThreadPriority.Highest;

            if(!UpdateThread.IsAlive)
                UpdateThread.Start();
        }

        public static void AddNewPath(Node StartingNode, Node TargetNode, Map map, bool DisableDiagonalPathfinding)
        {
            AstarQueue.Enqueue(new Astar(StartingNode, TargetNode, map, DisableDiagonalPathfinding));
        }

        static void Update()
        {
            while (true)
            {
                for (int i = 0; i < AstarQueue.Count; i++)
                {
                    Astar TempAstar;

                    AstarQueue.ToList()[i].FindPath();
                    AstarQueue.TryDequeue(out TempAstar);
                    AstarResults.Enqueue(TempAstar);
                    
                    Thread.Sleep(5);
                }
            }
        }
    }
}
Edited by FantasyVII

Share this post


Link to post
Share on other sites

I'm not a C# programmer, and even to me that code is clearly not thread safe.

 

Mistakes I've noticed in the Update() function:

- You're using a for loop to go through the queue. Since the count can change at any time due to other threads this is just wrong. Don't use .Count at all.

- Calling ToList() is going to be bad, for the same reason. The only way you should be examining the list contents is via TryDequeue().

- You ignore the return value from TryDequeue(). This is bad because if it's false you've not got a valid object to work with - the list is empty.

- The code only sleeps when there's an item in the list. If it's empty the thread eats 100% of one CPU core for no reason. You want the exact opposite of that.

- Looping calling Thread.Sleep() is a bad way to wait for work anyway. You want to use some sort of blocking wait function, which may require a different container.

 

There's also no sign of any test code. Even if you think you've got threaded code right, it's generally a good idea to do some stress testing, just in case you missed something.

 

My recommendation would be to avoid using threads until you have more experience. They are difficult to use correctly, even for experienced programmers, and modern CPUs run code fast enough on a single core that you can easily get away without them.

Share this post


Link to post
Share on other sites

I'm not a C# programmer, and even to me that code is clearly not thread safe.
 
Mistakes I've noticed in the Update() function:
- You're using a for loop to go through the queue. Since the count can change at any time due to other threads this is just wrong. Don't use .Count at all.
- Calling ToList() is going to be bad, for the same reason. The only way you should be examining the list contents is via TryDequeue().
- You ignore the return value from TryDequeue(). This is bad because if it's false you've not got a valid object to work with - the list is empty.
- The code only sleeps when there's an item in the list. If it's empty the thread eats 100% of one CPU core for no reason. You want the exact opposite of that.
- Looping calling Thread.Sleep() is a bad way to wait for work anyway. You want to use some sort of blocking wait function, which may require a different container.
 
There's also no sign of any test code. Even if you think you've got threaded code right, it's generally a good idea to do some stress testing, just in case you missed something.
 
My recommendation would be to avoid using threads until you have more experience. They are difficult to use correctly, even for experienced programmers, and modern CPUs run code fast enough on a single core that you can easily get away without them.

  
Thank you for the tips, that was really helpfully. Here is an update version of my code. I think I covered all your points from my understanding. not sure though.

 

- Looping calling Thread.Sleep() is a bad way to wait for work anyway. You want to use some sort of blocking wait function, which may require a different container.


Any ideas on how to do that?

 

 

 

I do apologize for my lack of knowledge. I feel like I'm just spamming code for other people to look at. again I apologize.

static void Update()
{
    while (true)
    {
        if (AstarQueue.Count > 0)
        {
            Astar TempAstar;

            AstarQueue.TryDequeue(out TempAstar);
            TempAstar.FindPath();
            AstarResults.Enqueue(TempAstar);
        }

        Thread.Sleep(5);
    }
}

Share this post


Link to post
Share on other sites
Your results queue is not a ConcurrentQueue, and yet you are expecting different threads to operate on it (one thread writes to it, another thread reads from it).

Thread.Sleep() is totally the wrong way to avoid throttling. You can end up in a situation where the thread is finishing a job right as another comes in, but it sleeps for 5 seconds before getting on to it. You can use something like an Event (there's an auto-reset and manual-reset variant that have very similar but different use cases) to replace both the Thread.Sleep and replace the need to ever query AstarQueue.Count (you should never ever need to do that; just use TryDeque and check the return value!). You can also use a Semaphore instead of an event.

Your thread loop would look something like:
 
run:
  wait on event
  reset event (if manual reset)
  while try deque job:
    run job
And your submission thread would simply be:
 
add job:
  enque job
  signal event
Some concurrent queues are fixed size and you might need to use a TryEnque and handle the failure case. I don't know .NET's library that well and don't know how their concurrent queue is implemented.

Share this post


Link to post
Share on other sites

Your results queue is not a ConcurrentQueue, and yet you are expecting different threads to operate on it (one thread writes to it, another thread reads from it).

Thread.Sleep() is totally the wrong way to avoid throttling. You can end up in a situation where the thread is finishing a job right as another comes in, but it sleeps for 5 seconds before getting on to it. You can use something like an Event (there's an auto-reset and manual-reset variant that have very similar but different use cases) to replace both the Thread.Sleep and replace the need to ever query AstarQueue.Count (you should never ever need to do that; just use TryDeque and check the return value!). You can also use a Semaphore instead of an event.

Your thread loop would look something like:
 

run:
  wait on event
  reset event (if manual reset)
  while try deque job:
    run job
And your submission thread would simply be:
 
add job:
  enque job
  signal event
Some concurrent queues are fixed size and you might need to use a TryEnque and handle the failure case. I don't know .NET's library that well and don't know how their concurrent queue is implemented.

 

 

 

Thank you, That really helped happy.png

 

Here is the last version of my code. I'm really hoping I fixed everything :)

namespace Engine.AI.Pathfinding
{
    public static class AstarManager
    {
        static Thread UpdateThread = new Thread(new ThreadStart(Update));
        static AutoResetEvent autoResetEvent = new AutoResetEvent(false);

        static ConcurrentQueue<Astar> AstarQueue = new ConcurrentQueue<Astar>();
        public static ConcurrentQueue<Astar> AstarResults = new ConcurrentQueue<Astar>();

        public static void Initialize()
        {
            UpdateThread.IsBackground = true;
            UpdateThread.Priority = ThreadPriority.Highest;

            if(!UpdateThread.IsAlive)
                UpdateThread.Start();
        }

        public static void AddNewPath(Node StartingNode, Node TargetNode, Map map, bool DisableDiagonalPathfinding)
        {
            AstarQueue.Enqueue(new Astar(StartingNode, TargetNode, map, DisableDiagonalPathfinding));
            autoResetEvent.Set();
        }

        static void Update()
        {
            while (true)
            {
                autoResetEvent.WaitOne();

                Astar TempAstar;

                AstarQueue.TryDequeue(out TempAstar);
                TempAstar.FindPath();
                AstarResults.Enqueue(TempAstar);
            }
        }
    }
}
Edited by FantasyVII

Share this post


Link to post
Share on other sites

This topic is 1103 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this