[Mono-list] this .net exe work for you?

Abe Gillespie abe.gillespie at gmail.com
Tue Oct 25 19:01:13 EDT 2005


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 <2a5gjx302 at sneakemail.com> 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  -  Mono-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-list
>


More information about the Mono-list mailing list