At 09:15 PM 24/10/2005 +0200, Jose Pascual wrote: >this .net exe work for you? > >(.net with winforms) > >regards
I have not tried running your EXE. However, I have decompiled it. For anyone who is interested: Plain text: http://israel.logiclrd.cx/ThreadTester.cs Highlighted: http://israel.logiclrd.cx/ThreadTester.cs.html Here is an analysis of your source code: --------------------------------------------------------------------------- using System; using System.Collections; using System.ComponentModel; using System.Drawing; using System.Threading; using System.Windows.Forms; public abstract class Shapes { // Methods public void CheckCoordinates() { if ((frm1.panel1.Size.Height - 20 < yVal) || (yVal <= 0)) directionY = directionY * -1; if ((frm1.panel1.Size.Width - 20 < xVal) || (xVal <= 0)) directionX = directionX * -1; } public abstract void paint(Graphics g); // Fields protected Color color; protected int directionX = 1; protected int directionY = 1; protected Form1 frm1; protected int height; protected int speed; protected int width; protected int xVal; protected int yVal; } public class Circle : Shapes { // Methods public Circle(int x, int y, int w, int h, Color c, Form1 f, int spd) { this.xVal = x; this.yVal = y; this.color = c; this.width = w; this.height = h; this.frm1 = f; this.speed = spd; } public override void paint(Graphics g) { try { Thread.Sleep(this.speed); g.DrawEllipse(new Pen(this.frm1.BackColor), this.xVal, this.yVal, this.width, this.height); --------------------------------------------------------------------------- Up to this point is okay. --------------------------------------------------------------------------- lock (typeof(Thread)) { this.xVal += this.directionX; this.yVal += this.directionY; base.CheckCoordinates(); } --------------------------------------------------------------------------- The above, however, is NOT okay. You don't even NEED the lock if the current thread is the only thread accessing those variables, but assuming the lock were required, you chose a very bad object to use. The 'Type' object for Thread is global across the entire AppDomain, which means all threads end up being forced to wait here while exactly one thread updates its own local variables. This is also a very dangerous object to lock, because it is entirely possible that the runtime might lock this object during some operation which suspends all threads -- say, garbage collection -- and with the thread holding the lock suspended, it could cause an unfixable deadlock. In the event that you do need to lock threads around a section of code (which will only be the case when the different threads are accessing the *same resource*), you need to use your own object to do the lock. You can lock the object itself, or, generally considered better practice, you can create an object specifically for locking: class Fuu { public readonly SyncRoot = new object(); public int Counter; public void Increment() { lock (SyncRoot) // a safe object to lock { Counter = Counter + 1; // no race conditions here } } } --------------------------------------------------------------------------- g.DrawEllipse(new Pen(this.color), this.xVal, this.yVal, this.width, this.height); } catch (Exception exception) {} --------------------------------------------------------------------------- Why are you catching all exceptions here and ignoring them? Are you perhaps *expecting* an exception to occur? The only reason that could be is that when testing, you encountered exceptions here. The correct course of action is to correct the code so that the exception doesn't happen. Very rarely does it make sense to ignore *all* exceptions. --------------------------------------------------------------------------- } } public class Rectangle : Shapes { // Methods public Rectangle(int x, int y, int w, int h, Color c, Form1 f, int spd) { this.xVal = x; this.yVal = y; this.color = c; this.width = w; this.height = h; this.frm1 = f; this.speed = spd; } public override void paint(Graphics g) { try { Thread.Sleep(this.speed); g.DrawRectangle(new Pen(this.frm1.BackColor), this.xVal, this.yVal, this.width, this.height); lock (typeof(Thread)) { this.xVal += this.directionX; this.yVal += this.directionY; base.CheckCoordinates(); } --------------------------------------------------------------------------- Same comment here as above. --------------------------------------------------------------------------- g.DrawRectangle(new Pen(this.color), this.xVal, this.yVal, this.width, this.height); } catch (Exception exception) {} --------------------------------------------------------------------------- ..and here too. --------------------------------------------------------------------------- } } public class Form1 : System.Windows.Forms.Form { public Form1() { // This call is required by the Windows Form Designer. InitializeComponent(); // Add any initialization after the InitializeComponent() call this.g = this.panel1.CreateGraphics(); --------------------------------------------------------------------------- A Graphics object on a System.Windows.Forms component uses an underlying object called a "DC" -- Device Context -- and these are limited across the entire system. The preferred practice is to create a new Graphics each time you need to draw and then release it when you're finished. This is especially pertinent because while the design principles of System.Windows.Forms do not encourage it, they also do not prevent the underlying Win32 handle from being destroyed & recreated, and if that happens, the DC, and thus the Graphics object, will be invalid. --------------------------------------------------------------------------- } #region "Windows Form Designer generated code" protected override void Dispose(bool disposing) { if (disposing && !(this.components == null)) this.components.Dispose(); base.Dispose(disposing); } // Required by the Windows Form Designer private Container components = null; //NOTE: The following procedure is required by the Windows Form Designer //It can be modified using the Windows Form Designer. //Do not modify it using the code editor. private Button cmdColor; private Button cmdExit; private Button cmdStart; private ComboBox comboBox1; private ComboBox comboBoxSpeed; private Label label1; public volatile Panel panel1; --------------------------------------------------------------------------- Why is this 'volatile'? The reason for 'volatile's existence is when multiple threads are accessing a variable *whose value is rapidly changing*. It defeats the processor's caching system so that the latest value is always retrieved; otherwise, a thread running in a tight loop might never get anything other than the first value it reads. Your 'panel1' variable is assigned only once, and this is done before any of the threads that need to read from it are even created. --------------------------------------------------------------------------- [System.Diagnostics.DebuggerStepThrough] private void InitializeComponent() { [snipped] } [STAThread] private static void Main() { Application.Run(new Form1()); } #endregion public ColorDialog c; private volatile Graphics g; --------------------------------------------------------------------------- Same thing here: this variable is only ever assigned once. As mentioned earlier, though, it shouldn't even exist. --------------------------------------------------------------------------- public static Color shapeColor = Color.Blue; private const int shapSize = 15; public static int threadCount = 0; private Hashtable threadHolder = new Hashtable(); private void StartThread() { Shapes shape; if (this.comboBox1.Text == "Rectangle") shape = new Rectangle(0, 0, shapSize, shapSize, Form1.shapeColor, this, Convert.ToInt32(this.comboBoxSpeed.Text.Trim())); else if (this.comboBox1.Text == "Circle") --------------------------------------------------------------------------- Here you should use comboBox1.SelectedIndex instead of .Text. While using .Text is arguably easier to read, you might possibly one day want to change the text, perhaps for localization ("Cercle" instead of "Circle" for French, say). Then again, you might one day want to change the order instead... --------------------------------------------------------------------------- shape = new Circle(0, 0, shapSize, shapSize, Form1.shapeColor, this, Convert.ToInt32(this.comboBoxSpeed.Text.Trim())); else shape = new Rectangle(0, 0, shapSize, shapSize, Form1.shapeColor, this, Convert.ToInt32(this.comboBoxSpeed.Text.Trim())); while (true) { try { shape.paint(this.g); --------------------------------------------------------------------------- Here is the core of your troubles. You have a DC to the window, and multiple threads are calling methods on that DC concurrently. This is extremely bad! While DCs are not bound to any specific threads like GUI widgets are, there is no guarantee of their re-entrancy. This isn't easy to fix; basically, your entire test program is based on a flawed assumption. Perhaps one way to fix it would be to use the "Invoke" method to marshal the paint calls onto the GUI thread. This would serialize the drawing operations. Another way to serialize them would be to lock an object (but NOT a system object! it must be an object that you created) that is common to the threads. For instance: class Fuu { // This object is a field variable of 'Fuu', not a local variable of the // thread procedure, and so it is common to all threads. object sync = new object(); public void ThreadProc() { while (true) { update_position(); lock (sync) { using (Graphics g = CreateGraphics()) paint(g); } } } } This example also shows the recommended use of CreateGraphics(). --------------------------------------------------------------------------- } catch (Exception exception) { --------------------------------------------------------------------------- I imagine you don't get this happening very often, since your paint functions already catch all exceptions and ignore them. --------------------------------------------------------------------------- Console.WriteLine("Exception in Form1 whileloop >> " + exception); return; } } } private void button1_Click(object sender, EventArgs e) { foreach (Thread thread in threadHolder.Values) if ((thread != null) && thread.IsAlive) thread.Abort(); --------------------------------------------------------------------------- I notice that farther down, you set the .IsBackground property of all the threads to 'true'. Calling .Abort() on them is therefore completely superfluous. As a matter of fact, .Abort() should virtually never be used; it is always more appropriate to find a way to notify the thread and let the thread end itself. When you call .Abort() on a thread, it throws an uncatchable ThreadAbortException. I say "uncatchable" because even if you do catch it, it is automatically rethrown at the end of the 'catch' block, so you can't stop its propagation. This does, then, have the effect of terminating the thread (provided it doesn't go into a loop in the 'catch' clause). However, when a thread is marked as a background thread, it is automatically killed off when all non-background threads are gone. Closing the main form will result in the "Application.Run()" call inside the 'Main' method to return. 'Main' itself will return, and the main thread will then terminate. The result will be that the only threads left will be background threads trying to draw to a non-existent form. When you expect this situation, the best course of action is to catch specifically the "ObjectDisposedException", which can be thrown whenever you call a method on the disposed Form (such as 'CreateGraphics()'). Unlike catching all exceptions, in this case you know specifically why the exception has been thrown and can stop trying to draw to the form. --------------------------------------------------------------------------- Form.ActiveForm.Close(); } private void button2_Click(object sender, EventArgs e) { Thread thread = new Thread(new ThreadStart(this.StartThread)); this.threadHolder.Add(threadCount++, thread); thread.Name = "Thread ID: " + Form1.threadCount; --------------------------------------------------------------------------- Not sure if this is your intent, but the thread's index in 'ThreadHolder' will be one less than the index in its 'Name' property... It would probably be more appropriate to use an ArrayList instead of a Hashtable in this situation, by the way, as you are never looking up a thread by its ID. The only thing you use the collection for is enumerating the threads and killing them. Enumerating a Hashtable is a very costly operation, while enumerating an ArrayList is about as fast as directly accessing an array. --------------------------------------------------------------------------- thread.IsBackground = true; thread.Start(); } private void cmdColor_Click(object sender, EventArgs e) { this.c = new ColorDialog(); this.c.ShowDialog(); Form1.shapeColor = this.c.Color; } private void comboBox1_SelectedIndexChanged(object sender, EventArgs e) { } private void Form1_Load(object sender, EventArgs e) { } } --------------------------------------------------------------------------- I know this happens when using the designer, that you hook events you didn't mean to hook, but it does look a little bit sloppy when you don't clean up after making that mistake. You can remove the binding by switching the Properties panel to the 'Events' (little lightning-bolt icon at the top of the panel), and then clicking once on the name of the method you want to unhook and pressing delete. The only other thing I noticed is that you aren't consistent in renaming your controls. This makes a big difference in how professional your code looks. For a test, it isn't that important, but to have "comboBox1", "label1", etc. and "cmdColor", "cmdExit", etc. in the same project looks bad. I also notice that the event handlers for two of the buttons are named differently ("button1" instead of "cmdExit", "button2" instead of "cmdStart"). Basically, this tells you that assigning the correct name to a control should be the first priority after adding it to the form, before you do anything else. If there are any parts of my explanation that were unclear to you, feel free to reply to me *off the list* and I'll elaborate. Good luck with your programming endeavours :-) Jonathan Gilbert _______________________________________________ Mono-list maillist - [email protected] http://lists.ximian.com/mailman/listinfo/mono-list
