[Mono-list] this .net exe work for you?
Jonathan Gilbert
2a5gjx302 at sneakemail.com
Tue Oct 25 17:01:29 EDT 2005
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
More information about the Mono-list
mailing list