ok, why does this suck?

[James]

hello,

trying to create what I thought would be a simple little tool... the nitty gritty is already taken care of by using the NetworkInformation's ping class. I am simply trying to wire it up to a windows form to create a ping utility that is supposed to be able to simultaneously ping several targets. I'm using the async method in the Ping class which I thought would be using threads other than the main UI thread, and so my UI would therefore remain responsive. I'm sure there are already tools out there that do what I'm after but I like creating my own. There is group at work that often is asked to basically throw a ping on a target and watch that it goes down, and then back up. They often have to do this for a handfull of targets at a time... and so they open multiple command windows and use the windows builtin ping.exe -t command. I'm just looking to make this app so there is one place they can do all their ping monitoring from.

so, I set out to use the existing.net ping class to do the actual pinging, a simple one form app using a listview control to display all the targets they have a ping on which lets them easily see an up or down status, and the ability to select one of the listview items to watch the real time replies come in via a textbox control.

I planned to implement start, stop, and of course remove buttons to manage the listview items, which should each represent a seperate target being pinged, each in a separate thread. The code I have so far is just enough to test out how the core funtionality will work, and it sucks. It works, but performs horribly, the UI does not remain responsive... very slow to react.

below is the code... whats wrong with it? I'm sure a lot considering I'm not that experienced in this kind of thing but would really like to learn how to make it work, correctly. Lots of questions, like do I even need to synchronize access to the listView control using the lock statement? With or without it the performance was still horrible, but no errors.

namespace Pingler
{
public partial class frmMain : Form
{
public frmMain()
{
InitializeComponent();
}

private object myLock = new object();


private void btnAdd_Click(object sender, EventArgs e)
{

// access to lstViewTargets must be synchronized since this is being designed
// to support multiple simultaneous pings
lock (myLock)
{

lstViewTargets.BeginUpdate();

stateObject stateBall = new stateObject();
stateBall.IP = txtIP.Text.Trim();
stateBall.MyListViewItem.Text = txtIP.Text.Trim();
stateBall.MyListViewItem.SubItems.Add("Waiting...");
stateBall.MyListViewItem.SubItems.Add("Waiting...");
lstViewTargets.Items.Add(stateBall.MyListViewItem);

lstViewTargets.EndUpdate();

// When the PingCompleted event is raised,
// the PingCompletedCallback method is called.
stateBall.pingSender.PingCompleted += new PingCompletedEventHandler(PingCompletedCallback);

// Send the ping asynchronously.
stateBall.pingSender.SendAsync(stateBall.IP, stateBall.Timeout, stateBall.Buffer, stateBall.Options, stateBall);

}

}

private void PingCompletedCallback(object sender, PingCompletedEventArgs e)
{

stateObject stateBall = (stateObject)e.UserState;
PingReply reply = e.Reply;

if (stateBall.FirstTime)
{

lock (myLock)
{
//first ping sent so initialize listview control with fisrt ping result
if (reply.Status == IPStatus.Success)
{
stateBall.MyListViewItem.SubItems[1].Text = "Up";
stateBall.MyListViewItem.StateImageIndex = 1;
}
else
{
stateBall.MyListViewItem.SubItems[1].Text = "Down";
stateBall.MyListViewItem.StateImageIndex = 0;
}

stateBall.MyListViewItem.SubItems[2].Text = reply.Status.ToString();

//flip first time tracker
stateBall.FirstTime = false;
}

}
else if (!stateBall.FirstTime)
{

//not first ping sent, only update gui if something has changed
if (stateBall.LastReply != reply.Status.ToString())
{
//status changed from last ping, update listview control
lock (myLock)
{
if (reply.Status == IPStatus.Success)
{
stateBall.MyListViewItem.SubItems[0].Text = "Up";
stateBall.MyListViewItem.StateImageIndex = 0;
}
else
{
stateBall.MyListViewItem.SubItems[0].Text = "Down";
stateBall.MyListViewItem.StateImageIndex = 1;
}

stateBall.MyListViewItem.SubItems[1].Text = reply.Status.ToString();
}
}

} //end of first time or not branches


//print detail if its the selected item
if (stateBall.MyListViewItem.Selected)
{

if (reply.Status == IPStatus.Success)
{
txtOutput.Text += Environment.NewLine;
txtOutput.Text += "Address: " + reply.Address.ToString() + " - ";
txtOutput.Text += "RoundTrip time: " + reply.RoundtripTime + " - ";
txtOutput.Text += "Time to live: " + reply.Options.Ttl + " - ";
txtOutput.Text += "Don't fragment: " + reply.Options.DontFragment + " - ";
txtOutput.Text += "Buffer size: " + reply.Buffer.Length;
}
else
{
txtOutput.Text += Environment.NewLine + "failed: " + reply.Status.ToString();
}

}

//store this reply's status in stateobject for use on next iteration
stateBall.LastReply = reply.Status.ToString();

//no need to ping it to death, pause 1.5 seconds
Thread.Sleep(1500);

if (stateBall.counter < 20)
{
stateBall.pingSender.SendAsync(stateBall.IP, stateBall.Timeout, stateBall.Buffer, stateBall.Options, stateBall);
stateBall.counter++;
}


}

}


public class stateObject
{
public Ping pingSender = new Ping();
public string Data;
public byte[] Buffer;
public int Timeout;
public PingOptions Options;
public string IP;
public int counter;

public ListViewItem MyListViewItem = new ListViewItem();
public bool PrintDetail;
public string LastReply;
public bool FirstTime;

public stateObject()
{
// Create a buffer of 32 bytes of data to be transmitted.
Data = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
Buffer = Encoding.ASCII.GetBytes(Data);

// Wait 6 seconds for a reply.
Timeout = 6000;

// Set options for transmission:
// The data can go through 64 gateways or routers
// before it is destroyed, and the data packet
// cannot be fragmented.
Options = new PingOptions(64, true);

counter = 0;

PrintDetail = false;
FirstTime = true;
LastReply = string.Empty;

}
}
}

 

[Pavel Minaev]

I planned to implement start, stop, and of course remove buttons to manage the listview items, which should each represent a seperate target being pinged, each in a separate thread. The code I have so far is just enough to test out how the core funtionality will work, and it sucks. It works, but performs horribly, the UI does not remain responsive... very slow to react.

below is the code... whats wrong with it? I'm sure a lot considering I'm not that experienced in this kind of thing but would really like to learn how to make it work, correctly. Lots of questions, like do I even need to synchronize access to the listView control using the lock statement? With or without it the performance was still horrible, but no errors.

One thing I can immediately tell regarding threads - you cannot work
with Windows Forms controls from threads other than the one from which
they were created - which is typically the thread which launched Main
(). If you need to update UI from another thread, you should use
Control.Invoke and Control.BeginInvoke as needed (see MSDN for
details).

Also, for operations running in the background on a different thread
that have to communicate some information to the UI thread, consider
using BackgroundWorker. It's quite well designed, and hides all the
low-level synchronization and data exchange from you.

 

[James]

thanks for the input, I appreciate it. Are there any disadvantages in using
the BackgroundWorker? If I know I am going to be needing 4 - 7 or so
threads, is BackgroundWorker still a good choice? I assume there is a 1 to 1
between BackgroundWorker and thread and so I would need to instantiate 4 - 7
or so BackgroundWorkers? is that ok? Obviously I have not read up on the
BackgroundWorker yet.

thanks again.

I planned to implement start, stop, and of course remove buttons to manage
the listview items, which should each represent a seperate target being
pinged, each in a separate thread. The code I have so far is just enough
to test out how the core funtionality will work, and it sucks. It works,
but performs horribly, the UI does not remain responsive... very slow to
react.

below is the code... whats wrong with it? I'm sure a lot considering I'm
not that experienced in this kind of thing but would really like to learn
how to make it work, correctly. Lots of questions, like do I even need to
synchronize access to the listView control using the lock statement? With
or without it the performance was still horrible, but no errors.

One thing I can immediately tell regarding threads - you cannot work
with Windows Forms controls from threads other than the one from which
they were created - which is typically the thread which launched Main
(). If you need to update UI from another thread, you should use
Control.Invoke and Control.BeginInvoke as needed (see MSDN for
details).

Also, for operations running in the background on a different thread
that have to communicate some information to the UI thread, consider
using BackgroundWorker. It's quite well designed, and hides all the
low-level synchronization and data exchange from you.

 

[James]

yes, you and Pavel are correct.

I have not done this in a while, and when I did do it a little some time ago
is was a console app that used multiple threads, so updating UI elements
from other threads is something I had not run into yet. Thats problem one,
and the second is as you said, the 1.5 second pause. I misunderstood how the
async method worked and I thought the callback function was running on a new
thread. So now I know its on the same thread as the UI I see why the
performance was so bad.

and I was trying to synchronize access to the listview control, which I now
know will be handled by control.invoke.

Basically, this is a ping utility, with a GUI, that allows several targets
to be pinged simultaneously, and I'm using a ListView control to show each
target and its status. The new plan is to make a new thread for each
target/pinger and use control.invoke to update the listView control.

so before I get started rewriting this I have one question, using the
control.invoke method to update UI from other threads, will I be able to
pass in an instance of a listViewItem to the delegate? or will I need to
change code to just pass in string info and have the delegate search for the
right listviewitem to update? Put another way, the form has a listview
control, each listviewItem in this control represents the status of a
continuous ping running in a thread other than the UI, is it possible to
instantiate the listViewItem with some preliminary info and add it to the
listView in the main UI, then pass the listviewItem to the method that runs
in the new thread, and have this new thread update the listViewItem and pass
the listViewItem back through to the control.invoke delegate to do the
update in the UI thread? ok, after thinking of how to ask this I think I
made it clear to myself this could not work.

so I would need to basically pass in 2 values to the control.invoke
delegate, 1 for locating the correct listviewItem to update, and the other
with the actual info to update?

[...] The code I have so far is just enough to test out how the core
funtionality will work, and it sucks. It works, but performs horribly,
the UI does not remain responsive... very slow to react.

You didn't post a concise-but-complete code sample, so there's no
practical way for anyone to see exactly what you're talking about.

But, I suspect that the 1.5 second sleep in your callback is at least
partially responsible for the problem. If you want to delay the next ping
by some amount of time, it would be better to use a one-shot timer instead
of blocking an entire thread for the period.

It's also not clear from your code why you're doing all that
synchronization ("lock (myLock)"). If you're trying to synchronize the
"stateBall" variable, then you ought to be locking around all uses of it,
not just some (or you might be able to get away with just making the
"FirstTime" field "volatile"). If you're trying to synchronize access to
the GUI object ("MyListViewItem"), then what you really should be doing is
using Control.Invoke() (as Pavel suggested), which will automatically
synchronize the code being invoked, by virtue of having it all run on the
same thread.

Pete

 

[Pavel Minaev]

thanks for the input, I appreciate it. Are there any disadvantages in using
the BackgroundWorker? If I know I am going to be needing 4 - 7 or so
threads, is BackgroundWorker still a good choice? I assume there is a 1 to 1
between BackgroundWorker and thread and so I would need to instantiate 4 - 7
or so BackgroundWorkers? is that ok? Obviously I have not read up on the
BackgroundWorker yet.

An instance of BackgroundWorker itself is very lightweight, so that's
not an issue.

One thing you should be aware of, however, is that BackgroundWorker
uses the .NET thread pool for its threads, and that has a maximum
thread number configured by default (see ThreadPool.SetMaxThreads).

 

[James]

ok, thanks.

thanks for the input, I appreciate it. Are there any disadvantages in
using
the BackgroundWorker? If I know I am going to be needing 4 - 7 or so
threads, is BackgroundWorker still a good choice? I assume there is a 1 to
1
between BackgroundWorker and thread and so I would need to instantiate 4 -
7
or so BackgroundWorkers? is that ok? Obviously I have not read up on the
BackgroundWorker yet.

An instance of BackgroundWorker itself is very lightweight, so that's
not an issue.

One thing you should be aware of, however, is that BackgroundWorker
uses the .NET thread pool for its threads, and that has a maximum
thread number configured by default (see ThreadPool.SetMaxThreads).

 

[James]

thanks Peter, I appreciate your curiosity and detailed help.