[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