Wow Jonathan, you're very generous. What to donate some of you copious amounts of time to my programming endeavours? :)
-Abe On 10/25/05, Jonathan Gilbert <[EMAIL PROTECTED]> wrote: > 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 > _______________________________________________ Mono-list maillist - [email protected] http://lists.ximian.com/mailman/listinfo/mono-list
