Jump to content
  • Advertisement
Sign in to follow this  
FantasyVII

Weird stuff happening when stepping through a threaded code

This topic is 1468 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?

 

 

 

 

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
Advertisement

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
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!