Why Does Creating This New Task Crash My Program?

  • (3 Pages)
  • +
  • 1
  • 2
  • 3

35 Replies - 4874 Views - Last Post: 13 July 2012 - 03:48 PM Rate Topic: -----

#1 adn258  Icon User is offline

  • D.I.C Addict

Reputation: 11
  • View blog
  • Posts: 761
  • Joined: 31-August 11

Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 02:04 AM

So I'm getting better with parallelism and threads and tasks etc. I know that tasks run in the background and I wanted to start a factory task (just experimenting) to have it gather text files to an array of strings. This far it either crashes does nothing and causes general all around problems and I get object instance errors. Is there something wrong with using Directory.GetFiles within a task since the task is in the background? I'm confused. Code Below. Thanks guys

string[] files;
        public Form1()
        {
            InitializeComponent();
        }

        private void btn_Browse_Click(object sender, EventArgs e)
        {

            using (FolderBrowserDialog fd = new FolderBrowserDialog())
            {
                fd.Description = "Selected Directory";
                if (fd.ShowDialog() == DialogResult.OK)
                {
                    TxtBx_DirectorySelect.Text = fd.SelectedPath;
                }
            }
        }

        private void btn_Calculate_Click(object sender, EventArgs e)
        {
            Task.Factory.StartNew(() =>
                {
                    files = Directory.GetFiles(TxtBx_DirectorySelect.Text, "*.txt", SearchOption.AllDirectories);
                    InvokeAction(() => progressBar1.Maximum = files.Length);
                });

           
        }

 private void InvokeAction(Action action)
        {
            if (InvokeRequired)
            {
                BeginInvoke(new MethodInvoker(action));
            }
            else
            {
                action();
            }
        }




Is This A Good Question/Topic? 0
  • +

Replies To: Why Does Creating This New Task Crash My Program?

#2 janne_panne  Icon User is offline

  • WinRT Dev
  • member icon

Reputation: 429
  • View blog
  • Posts: 1,047
  • Joined: 09-June 09

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 04:22 AM

What errors are you receiving? I didn't receive any errors, although you seem to access TxtBx_DirectorySelect.Text property inside the task without invoking, which might result to a illegal cross thread exception.
Was This Post Helpful? 1
  • +
  • -

#3 tlhIn`toq  Icon User is online

  • Please show what you have already tried when asking a question.
  • member icon

Reputation: 5440
  • View blog
  • Posts: 11,672
  • Joined: 02-June 10

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 06:19 AM

I agree with Janne. Putting the work of your methods inside the button_click event handlers is pretty sloppy. It means you can't send parameters like you should. Instead have the button_click call a method to do the work and send the information you need (like the file path) as a parameter. See common tip #4 below:

Some of my common tips (some may apply more than others to your specific style):
  • You have to program as if everything breaks, nothing works, the cyberworld is not perfect, the attached hardware is flakey, the network is slow and unreliable, the harddrive is about to fail, every method will return an error and every user will do their best to break your software. Confirm everything. Range check every value. Make no assumptions or presumptions.

  • Take the extra 3 seconds to rename your controls each time you drag them onto a form. The default names of button1, button2... button54 aren't very helpful. If you rename them right away to something like btnOk, btnCancel, btnSend etc. it helps tremendously when you make the methods for them because they are named after the button by the designer.btnSend_Click(object sender, eventargs e) is a lot easier to maintain than button1_click(object sender, eventargs e)

  • You aren't paying for variable names by the byte. So instead of variables names of a, b, c go ahead and use meaningful names like index, timeOut, row, column and so on. You should avoid 'T' for the timer. Amongst other things 'T' is commonly used throughout C# for Type and this will lead to problems. There are naming guidelines you should follow so your code confirms to industry standards. It makes life much easier on everyone around you, including those of us here to help. If you start using the standards from the beginning you don't have to retrain yourself later.
    You might want to look at some of the naming guidelines. Its a lot easier to start with good habits than to break bad habits later and re-learn.



  • Try to avoid having work actually take place in GUI control event handlers. It is better to have the GUI handler call other methods so those methods can be reused and make the code more readable.
    Spoiler



  • Don't replace lines of code that don't work. Instead comment them out and put your new attemps below that. This will keep you from re-trying the same ideas over and over. Also, when you come back to us saying "I've tried this 100 different ways and still can't get it", we can actually see what you tried. So often a failed attempt is very very close and just needs a little nudge in the right direction. So if we can say "See what you did in attempt 3... blah blah" it helps a lot

    Spoiler

    If you are using Visual Studio you can select a block of lines and hit control+k control+c (Kode Comment) to comment it out. control+k control+u (Kode Uncomment) to uncomment a selected block.

This post has been edited by tlhIn`toq: 06 July 2012 - 06:23 AM

Was This Post Helpful? 1
  • +
  • -

#4 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2241
  • View blog
  • Posts: 9,412
  • Joined: 29-May 08

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 07:11 AM

adn258 you need to think functionally and isolation. A method / task should act only on it inputs and nothing else.
Signal notifiable changes in internal state via raising an event.
Was This Post Helpful? 1
  • +
  • -

#5 adn258  Icon User is offline

  • D.I.C Addict

Reputation: 11
  • View blog
  • Posts: 761
  • Joined: 31-August 11

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 12:46 PM

Thanks for your help guys. I figured out the error. The error was caused by. Getting the files task was not completing before the parallel of the foreach on each of these files so I guess you have to use Task.Wait for that purpose.

That said this whole threading,tasks, parallelism I again emphasize I'm just trying to learn and get a grip with. It has been a frustrating experience to say the least lol. In any case here is my code below. Besides my naming conventions which I know need improvement I would love it if someone would just look at this and tell me if it's acceptable in quality for finding blank documents. This is just something I'm doing for experience.

Thanks Guys I ended up with this Code AND I AM TRYING TO USE MOSTLY METHODS UNDER A UI METHOD AS SUGGESTED...THAT SAID I WANT TO KNOW IF THESE METHODS ARE ACCEPTABLE AND MY ALGORITHM.

CODE

 private void btn_Browse_Click(object sender, EventArgs e)
        {

            using (FolderBrowserDialog fd = new FolderBrowserDialog())
            {
                fd.Description = "Selected Directory";
                if (fd.ShowDialog() == DialogResult.OK)
                {
                    TxtBx_DirectorySelect.Text = fd.SelectedPath;
                }
            }
        }

        private void btn_Calculate_Click(object sender, EventArgs e)
        {
            Reset();
            string[] thefiles = { };
            Task getfiles = new Task(() => thefiles = GetFileNum());
            getfiles.Start();
            Task.WaitAny(getfiles);
            if (thefiles.Length < 1)
            { return; }

            PCalculateBlankTxt(thefiles);
          
        }

        private void PCalculateBlankTxt(string[] files)
        {
            Task.Factory.StartNew(() =>
                {
                    Parallel.ForEach(files, f =>
                       {
                           InvokeAction(() => progressBar1.Value++);
                           if (File.ReadAllText(f).Trim().Length == 0)
                           {
                               InvokeAction(() => LstBx_Files.Items.Add(f));
                           }
                       });

                });

        }

        private string[] GetFileNum()
        {
            string[] files = { };
            files = Directory.GetFiles(TxtBx_DirectorySelect.Text, "*.txt", SearchOption.AllDirectories);
            InvokeAction(() => progressBar1.Maximum = files.Length);
            if (files.Length > 0)
            {
                return files;
            }
            return null;
        }

        private void InvokeAction(Action action)
        {
            if (InvokeRequired)
            {
                BeginInvoke(new MethodInvoker(action));
            }
            else
            {
                action();
            }
        }

        private void Reset()
        {
            LstBx_Files.Items.Clear();
            progressBar1.Value = 0;
        }



EDIT

Also would making another method and using ANOTHER TASK Be even better for the button calculate this is just a small edit to the above code see

 private void btn_Calculate_Click(object sender, EventArgs e)
        {
            //start calculation method on new task
            Task.Factory.StartNew(() => CalculationOfFiles());
          
        }

        private void CalculationOfFiles()
        {
            Reset();
            string[] thefiles = { };
            using (Task getfiles = new Task(() => thefiles = GetFileNum()))
            {
                getfiles.Start();
                Task.WaitAny(getfiles);
                if (thefiles.Length < 1)
                { return; }
                PCalculateBlankTxt(thefiles);
            }
        }



Doing the same thing only in another method and task. Is this better? What's the difference?

Thanks

This post has been edited by adn258: 06 July 2012 - 01:01 PM

Was This Post Helpful? 0
  • +
  • -

#6 Skydiver  Icon User is offline

  • Code herder
  • member icon

Reputation: 3481
  • View blog
  • Posts: 10,733
  • Joined: 05-May 12

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 01:10 PM

GetFileNum() doesn't actually get the number of files... It seems to get all the file names. You should name your functions to match what they really do.

So your definition of a blank file is a file that contains only white space?

To me it just seems really expensive to read in an entire file, and trim away the leading and trailing whitespace, and then determine whether it is blank or not. To me, it would be quicker to open the file and and start streaming forward. If you encounter the first non-whitespace, then the file is not blank. If you make it all the way to the end then it must be blank. That way, when you open a 200MB image file, you can flag it as non-blank right away, instead of opening up the file, reading in 200MB into memory, and then having Trim() allocate another 200MB or so, and then declaring it non-blank when the Trim() hardly trims anything.

I'm a big proponent of keeping the data model separate from the UI, but I think it'll require more extensive changes to do that with your current code structure.
Was This Post Helpful? 1
  • +
  • -

#7 adn258  Icon User is offline

  • D.I.C Addict

Reputation: 11
  • View blog
  • Posts: 761
  • Joined: 31-August 11

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 02:34 PM

View PostSkydiver, on 06 July 2012 - 01:10 PM, said:

GetFileNum() doesn't actually get the number of files... It seems to get all the file names. You should name your functions to match what they really do.

So your definition of a blank file is a file that contains only white space?

To me it just seems really expensive to read in an entire file, and trim away the leading and trailing whitespace, and then determine whether it is blank or not. To me, it would be quicker to open the file and and start streaming forward. If you encounter the first non-whitespace, then the file is not blank. If you make it all the way to the end then it must be blank. That way, when you open a 200MB image file, you can flag it as non-blank right away, instead of opening up the file, reading in 200MB into memory, and then having Trim() allocate another 200MB or so, and then declaring it non-blank when the Trim() hardly trims anything.

I'm a big proponent of keeping the data model separate from the UI, but I think it'll require more extensive changes to do that with your current code structure.


but isn't that the case with the modified example since I use
Task.Factory.StartNew(() => CalculationOfFiles());

I thought that is what this is doing? is keeping the UI separate? That said this isn't looking for blank image files it's just using the txt files; that said I can see your point that it might be slightly fast to continue or return in the case of a parallel loop as soon as a char in a txt file is reached that might be slightly faster and all that jazz; I'd be interested in seeing your algorithm example for that?

That all said though remember I'm doing this for practice in parallelzation not to make the best algorithm in the world for the files method I just want to focus on parallel (though I'm not saying that's not important too). Thanks man for your help.
Was This Post Helpful? 0
  • +
  • -

#8 Skydiver  Icon User is offline

  • Code herder
  • member icon

Reputation: 3481
  • View blog
  • Posts: 10,733
  • Joined: 05-May 12

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 03:12 PM

View Postadn258, on 06 July 2012 - 02:34 PM, said:

I thought that is what this is doing? is keeping the UI separate? That said this isn't looking for blank image files it's just using the txt files; that said I can see your point that it might be slightly fast to continue or return in the case of a parallel loop as soon as a char in a txt file is reached that might be slightly faster and all that jazz; I'd be interested in seeing your algorithm example for that?


Replace:
if(File.ReadAllText(f).Trim().Length == 0)


with
if(IsFileAllWhiteSpace(f))

:

bool IsFileAllWhiteSpace(string fileName)
{
    using(StreamReader streamReader = new StreamReader(fileName))
    {
        int ch = streamReader.Read();
        while(ch != -1)
        {
            if (!Char.IsWhiteSpace((char) ch))
                return false;
            ch = streamReader.Read();
        }
    }
    return true;
}


Was This Post Helpful? 1
  • +
  • -

#9 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2241
  • View blog
  • Posts: 9,412
  • Joined: 29-May 08

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 03:17 PM

Nope. Tasks ain't magical.
      private string[] GetFileNum()
       {
           string[] files = { };
           files = Directory.GetFiles(TxtBx_DirectorySelect.Text, "*.txt", SearchOption.AllDirectories);
           InvokeAction(() => progressBar1.Maximum = files.Length);
           if (files.Length > 0)
           {
               return files;
           }
           return null;
       }



You access controls TxtBx_DirectorySelect and progressBar1.

Cutting all the crap out, the code below just acts on it input parameter(s) DirSelect

      private string[] GetFileNum(String DirSelect)
       {
           return Directory.GetFiles(DirSelect, "*.txt", SearchOption.AllDirectories);
       }



No GUI controls involved.


task<String[]> t1 = New task<String[]>( (String DirSelect)=> GetFileNumber(DirSelect) );
t1.Start
Task,WaitAll(ti)
// code to check task ended successfully etc.


This post has been edited by AdamSpeight2008: 06 July 2012 - 03:25 PM

Was This Post Helpful? 1
  • +
  • -

#10 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2241
  • View blog
  • Posts: 9,412
  • Joined: 29-May 08

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 03:24 PM

Skydiver
Inside of using block don't do early returns as the stream isn't disposed of, practice single point of exit techniques.

bool IsFileAllWhiteSpace(string fileName)
{   
    bool ok = true; // Assumed //
    using(StreamReader streamReader = new StreamReader(fileName))
    {
        while !(streamReader.EndOfStream) && ok
        {
            if (!Char.IsWhiteSpace((char) streamReader.Read()))
            { ok=false; }
       
        }
    }
    return ok;
}



This post has been edited by AdamSpeight2008: 06 July 2012 - 03:26 PM

Was This Post Helpful? 0
  • +
  • -

#11 Skydiver  Icon User is offline

  • Code herder
  • member icon

Reputation: 3481
  • View blog
  • Posts: 10,733
  • Joined: 05-May 12

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 04:07 PM

View PostAdamSpeight2008, on 06 July 2012 - 03:24 PM, said:

Skydiver
Inside of using block don't do early returns as the stream isn't disposed of, practice single point of exit techniques.

bool IsFileAllWhiteSpace(string fileName)
{   
    bool ok = true; // Assumed //
    using(StreamReader streamReader = new StreamReader(fileName))
    {
        while !(streamReader.EndOfStream) && ok
        {
            if (!Char.IsWhiteSpace((char) streamReader.Read()))
            { ok=false; }
       
        }
    }
    return ok;
}




Although I believe in single point of exit, I only apply the principle if the function is more than 30 lines long. Anything less, everything is visible on screen. Besides, in production code do you actually have a single point of after all the parameter validation without goto's? Or do you not count thrown exceptions as points of exit? Or do you have a chain of code that looks like
void FunctionMe(int param1, string param2, Xyz xyz)
{
    bool valid = true;
    string message = null;

    // parameter validation
    if (valid && param1 < 0)
    {
        valid = false;
        message = "Invalid param1";
    }
    if (valid && String.IsNullOrEmpty(param2))
    {
        valid = false;
        message = "Invalid param2";
    }
    if (valid && null != xyz && xyz.IsInitialized())
    {
        valid = false;
        message = "Invalid xyz";
    }

    // real work
    if (valid)
    {
        if (!xyz.Foo())
            valid = false;
    }

    // Single point of exit
    if (!valid && !String.IsNullOrEmpty(message))
    {
        throw new ApplicationException(message);
    }

    return valid;
}



Can you show where in the documentation that says that the stream is not disposed off when returning out of the using. I present the following quick and dirty code:
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace usingIDisposable
{
    class MyResource : IDisposable
    {
        string _id;
        public MyResource(string id)
        {
            _id = id;
            Console.WriteLine("MyResource(): _id = {0}", _id);
        }

        ~MyResource()
        {
            Console.WriteLine("~MyResource(): _id = {0}", _id);
            Dispose(false);
        }

        public void Dispose()
        {
            Console.WriteLine("MyResource.Dispose(): _id = {0}", _id);
            Dispose(true);
            GC.SuppressFinalize(this);
        }

        protected virtual void Dispose(bool disposing)
        {
            Console.WriteLine("MyResource.Dispose({1}): _id = {0}", _id, disposing);
        }
    }

    class Program
    {
        static void DoEarlyExit()
        {
            Console.WriteLine("DoEarlyExit enter");
            using (var res = new MyResource("Early"))
            {
                for (int i = 0; i < 20; ++i)
                {
                    if (i == 10)
                        return;
                }
            }
            Console.WriteLine("DoEarlyExit leave");
        }

        static void DoSinglePointExit()
        {
            Console.WriteLine("DoSinglePointExit enter");
            using (var res = new MyResource("SinglePoint"))
            {
                for (int i = 0; i < 20; ++i)
                {
                    if (i == 10)
                        break;
                }
            }
            Console.WriteLine("DoSinglePointExit leave");
        }

        static void Main(string[] args)
        {
            DoEarlyExit();
            DoSinglePointExit();
            Console.WriteLine("Done.");
        }
    }
}



which produces the following output
DoEarlyExit enter
MyResource(): _id = Early
MyResource.Dispose(): _id = Early
MyResource.Dispose(True): _id = Early
DoSinglePointExit enter
MyResource(): _id = SinglePoint
MyResource.Dispose(): _id = SinglePoint
MyResource.Dispose(True): _id = SinglePoint
DoSinglePointExit leave
Done.



Notice that the Dispose() is called appropriately by the early return.

Additionally see the StreamReader Dispose code:
// Inherited from TextReader:
public void Dispose()
{
    this.Dispose(true);
    GC.SuppressFinalize(this);
}

// In StreamReader:
protected override void Dispose(bool disposing)
{
    try
    {
        if ((this.Closable && disposing) && (this.stream != null))
        {
            this.stream.Close();
        }
    }
    finally
    {
        if (this.Closable && (this.stream != null))
        {
            this.stream = null;
            this.encoding = null;
            this.decoder = null;
            this.byteBuffer = null;
            this.charBuffer = null;
            this.charPos = 0;
            this.charLen = 0;
            base.Dispose(disposing);
        }
    }
}



Also: http://brendan.enric...-Statement.aspx

The times I have seen issues with returns in the middle of using is when Dispose() was not implemented properly.
Was This Post Helpful? 1
  • +
  • -

#12 Skydiver  Icon User is offline

  • Code herder
  • member icon

Reputation: 3481
  • View blog
  • Posts: 10,733
  • Joined: 05-May 12

Re: Why Does Creating This New Task Crash My Program?

Posted 06 July 2012 - 04:25 PM

And as final proof... MSDN documents the using statement as translation into a try-finally{} block where the finally block includes the Dispose() call:
http://msdn.microsof...y/yh598w02.aspx

So to show that a return call with in a try-finally block still calls the finally:
        static void DoTryFinally()
        {
            Console.WriteLine("DoTryFinally enter");
            try
            {
                Console.WriteLine("Try");
                return;
            }
            finally
            {
                Console.WriteLine("Finally");
            }
            Console.WriteLine("DoTryFinally leave");
        }


which produces the following output:
DoTryFinally enter
Try
Finally


Showing that the finally block is called even with return statement as the second line within the try block.
Was This Post Helpful? 1
  • +
  • -

#13 adn258  Icon User is offline

  • D.I.C Addict

Reputation: 11
  • View blog
  • Posts: 761
  • Joined: 31-August 11

Re: Why Does Creating This New Task Crash My Program?

Posted 07 July 2012 - 12:23 AM

Okay now I'm getting confused because like skydiver I always thought that when you use using( even if you return before the last brace the objects ARE DISPOSED, and as best as I understand MSDN'S documentation on that they indeed ARE DISPOSED in a method that is returned from early like that. I really want this was 100% confirmed though and will run my own tests but seriously is it always disposed? I thought MSDN made it clear if you use using it WILL get disposed one way or another from exceptions or even returns from using period because of the finally statement etc. I have been writing my methods with early returns like that NOT using a single point of exit for a long time now with C#

This post has been edited by adn258: 07 July 2012 - 12:34 AM

Was This Post Helpful? 0
  • +
  • -

#14 CodingSup3rnatur@l-360  Icon User is offline

  • D.I.C Addict
  • member icon

Reputation: 991
  • View blog
  • Posts: 971
  • Joined: 30-September 10

Re: Why Does Creating This New Task Crash My Program?

Posted 07 July 2012 - 03:50 AM

A using statement is just a try-finally block, as Skydiver said. Therefore, a return statement in a using block will definitely mean that the object is disposed of. That is 100%, no question, so don't worry about that.

However, it is worth knowing that there are scenarios where a finally block is not executed (aside from powering off the computer, of course ;)), and therefore, in the case of a using block, Dispose() is not called on the resource you are 'using'.

Here is some test code to illustrate some of these scenarios:

Spoiler


If you run each of those methods without the debugger attached (Ctrl + F5 in Visual Studio), you should see that Dispose() isn't called at all for any of the methods, even though I have made careful use of using blocks.

Also, killing your process from task manager will obviously have the same effect as Test4().

However, for most normal purposes, a finally block will always run. In the case of the using statement specifically, these fringe cases won't usually be a problem, as they all cause the process to exit, allowing Windows to clean up any resource that were in use by the process, and didn't get cleaned up. Therefore, this:

Quote

if you use using it WILL get disposed one way or another


is generally correct, even with these fringe cases. However, it does depend to a degree on what the resource you are wanting to 'dispose of' is. There are cases where resources may not be 'cleaned up' as you expect (even by the operating system) if one of the above scenarios occur, meaning Dispose() isn't called. See here, and here, for possible examples.


Try not to get hung up on these cases where finally blocks aren't run though, just be aware of their existence, and that saying a finally block always executes isn't strictly true :)

EDIT:

As for the single point of exit argument, I can definitely see the point in unmanaged languages like C that demand manual memory management. Having a single point of exit, by which point all temporary heap memory allocated within a function must have been freed, brings big advantages. You can get in a huge mess very quickly with multiple exit points in C.

However, in modern managed languages with exceptions and such like, I'm not sold at all. I tend to agree with Skydiver on that subject. Plus if a method gets to 30+ lines long as he mentioned, it may often be a good idea to split it into a number of smaller methods anyway (depending on the scenario obviously).

I would say that, in C#, unless you genuinely see value in it (which I don't, and I think it can adversely affect readability and maintainability), don't feel obliged to stick to the single exit point rule. At the end of the day, that's just my opinion though :)

This post has been edited by CodingSup3rnatur@l-360: 07 July 2012 - 08:23 AM

Was This Post Helpful? 1
  • +
  • -

#15 AdamSpeight2008  Icon User is offline

  • MrCupOfT
  • member icon


Reputation: 2241
  • View blog
  • Posts: 9,412
  • Joined: 29-May 08

Re: Why Does Creating This New Task Crash My Program?

Posted 07 July 2012 - 08:58 AM

Skydiver Single point of exit for the Using statement(s), not the entire method.
Was This Post Helpful? 1
  • +
  • -

  • (3 Pages)
  • +
  • 1
  • 2
  • 3