Code Review needed
-
Consider this C# code: /////// public class Worker { public ManualResetEvent MyEvent; private Thread t; private string message = "nothing now"; //Constructor public Worker(string name) { t = new Thread(new ThreadStart(this.Work)); t.Name = "Worker: " + name; t.Start(); //I don't like the following line: while(t.ThreadState == ThreadState.Unstarted){} } //Property public string Message { get{return message;} set{message = value;} } //Thread function public void Work() { MyEvent = new ManualResetEvent(false); for(;;)//Loop forever { MyEvent.Reset(); MyEvent.WaitOne(); //Wait until "MyEvent" is Set Console.Write("Message:" + message + ".\r\n"); } } //terminate the thread public void kill() { t.Abort(); } } ////// Worker w = new Worker("Thread 1"); w.Message="Something"; w.MyEvent.Set(); .... w.Message="Something else"; w.MyEvent.Set(); ... w.kill(); ---------------------------------------------------------------- Q1: Is encapsulating the thread in the class like this bad practice? Q2: Assuming that there is nothing wrong with encapsulating the thread, is there anyway I can terminate it in a destructor? I would like to see this thread die when the instance of the class goes out of scope. I think explicitly calling the kill method is lame. (Or do I just have to get used to the way things are destroyed in C#?) Q3: What is the “approved” method of waiting for a thread to start? i.e. is there anything better than this: while(t.ThreadState == ThreadState.Unstarted){} ----- Any other comments would be appreciated. Thanks, -Kevin
-
Consider this C# code: /////// public class Worker { public ManualResetEvent MyEvent; private Thread t; private string message = "nothing now"; //Constructor public Worker(string name) { t = new Thread(new ThreadStart(this.Work)); t.Name = "Worker: " + name; t.Start(); //I don't like the following line: while(t.ThreadState == ThreadState.Unstarted){} } //Property public string Message { get{return message;} set{message = value;} } //Thread function public void Work() { MyEvent = new ManualResetEvent(false); for(;;)//Loop forever { MyEvent.Reset(); MyEvent.WaitOne(); //Wait until "MyEvent" is Set Console.Write("Message:" + message + ".\r\n"); } } //terminate the thread public void kill() { t.Abort(); } } ////// Worker w = new Worker("Thread 1"); w.Message="Something"; w.MyEvent.Set(); .... w.Message="Something else"; w.MyEvent.Set(); ... w.kill(); ---------------------------------------------------------------- Q1: Is encapsulating the thread in the class like this bad practice? Q2: Assuming that there is nothing wrong with encapsulating the thread, is there anyway I can terminate it in a destructor? I would like to see this thread die when the instance of the class goes out of scope. I think explicitly calling the kill method is lame. (Or do I just have to get used to the way things are destroyed in C#?) Q3: What is the “approved” method of waiting for a thread to start? i.e. is there anything better than this: while(t.ThreadState == ThreadState.Unstarted){} ----- Any other comments would be appreciated. Thanks, -Kevin
ke5in wrote: Q1: Is encapsulating the thread in the class like this bad practice? No. It looks reasonable enough. ke5in wrote: Assuming that there is nothing wrong with encapsulating the thread, is there anyway I can terminate it in a destructor? I would like to see this thread die when the instance of the class goes out of scope. I think explicitly calling the kill method is lame. (Or do I just have to get used to the way things are destroyed in C#?) Since GC is non-deterministic in .NET you could never be sure when the destructor of your class is run. If you don't mind the thread hanging around for some time after the class goes out of scope you could put the kill() call in the destructor. Although i wouldn't advise it. May the Source be with you Sonork ID 100.9997 sijinjoseph
-
Consider this C# code: /////// public class Worker { public ManualResetEvent MyEvent; private Thread t; private string message = "nothing now"; //Constructor public Worker(string name) { t = new Thread(new ThreadStart(this.Work)); t.Name = "Worker: " + name; t.Start(); //I don't like the following line: while(t.ThreadState == ThreadState.Unstarted){} } //Property public string Message { get{return message;} set{message = value;} } //Thread function public void Work() { MyEvent = new ManualResetEvent(false); for(;;)//Loop forever { MyEvent.Reset(); MyEvent.WaitOne(); //Wait until "MyEvent" is Set Console.Write("Message:" + message + ".\r\n"); } } //terminate the thread public void kill() { t.Abort(); } } ////// Worker w = new Worker("Thread 1"); w.Message="Something"; w.MyEvent.Set(); .... w.Message="Something else"; w.MyEvent.Set(); ... w.kill(); ---------------------------------------------------------------- Q1: Is encapsulating the thread in the class like this bad practice? Q2: Assuming that there is nothing wrong with encapsulating the thread, is there anyway I can terminate it in a destructor? I would like to see this thread die when the instance of the class goes out of scope. I think explicitly calling the kill method is lame. (Or do I just have to get used to the way things are destroyed in C#?) Q3: What is the “approved” method of waiting for a thread to start? i.e. is there anything better than this: while(t.ThreadState == ThreadState.Unstarted){} ----- Any other comments would be appreciated. Thanks, -Kevin
My answer would be to implement events in the threaded class (Started and Exit) and responding to them in the main program. The library I use for my IRC client makes use of threaded classes like this (i think, as far as i can see anyways :)) Have a look at http://thresher.sourceforge.net[^] The library runs rock solid, so I would assume this is the correct way to do it. Hope this Helps :) PS: ke5in, plz dont tick respond when receiving a reply, if you dont have a working email address ( or fix tour email address). Err: To: "ke5in" Mail returned - user not found. MYrc : A .NET IRC client with C# Plugin Capabilities. See http://sourceforge.net/projects/myrc for more info. :-D
-
Consider this C# code: /////// public class Worker { public ManualResetEvent MyEvent; private Thread t; private string message = "nothing now"; //Constructor public Worker(string name) { t = new Thread(new ThreadStart(this.Work)); t.Name = "Worker: " + name; t.Start(); //I don't like the following line: while(t.ThreadState == ThreadState.Unstarted){} } //Property public string Message { get{return message;} set{message = value;} } //Thread function public void Work() { MyEvent = new ManualResetEvent(false); for(;;)//Loop forever { MyEvent.Reset(); MyEvent.WaitOne(); //Wait until "MyEvent" is Set Console.Write("Message:" + message + ".\r\n"); } } //terminate the thread public void kill() { t.Abort(); } } ////// Worker w = new Worker("Thread 1"); w.Message="Something"; w.MyEvent.Set(); .... w.Message="Something else"; w.MyEvent.Set(); ... w.kill(); ---------------------------------------------------------------- Q1: Is encapsulating the thread in the class like this bad practice? Q2: Assuming that there is nothing wrong with encapsulating the thread, is there anyway I can terminate it in a destructor? I would like to see this thread die when the instance of the class goes out of scope. I think explicitly calling the kill method is lame. (Or do I just have to get used to the way things are destroyed in C#?) Q3: What is the “approved” method of waiting for a thread to start? i.e. is there anything better than this: while(t.ThreadState == ThreadState.Unstarted){} ----- Any other comments would be appreciated. Thanks, -Kevin
ke5in wrote: Q1: Is encapsulating the thread in the class like this bad practice? Not particularly, but why not have a seperate class containing an event, replace the ManualRaiseEvent with this class and then have an event handler in your Worker class? I tend to believe in threading when I want two things to happen simultaneously (maybe if your Worker thread was continuously updating a database or an on-screen display while the program was off doing something else) rather than just waiting for something to happen. That's what events are for. ke5in wrote: Q2: Assuming that there is nothing wrong with encapsulating the thread, is there anyway I can terminate it in a destructor? I would like to see this thread die when the instance of the class goes out of scope. I think explicitly calling the kill method is lame. (Or do I just have to get used to the way things are destroyed in C#?) Nothing wrong with doing it this way... you might want to call it "Dispose" though, that's the C#/VB.NET standard for killing something without waiting for it to be deconstructed. ke5in wrote: Q3: What is the “approved” method of waiting for a thread to start? i.e. is there anything better than this: while(t.ThreadState == ThreadState.Unstarted){} Seems like a good method to me. You might want to Sleep(10) in the loop though, just so other programs can get at your processor while you're waiting. Paul