No offense intended, but there are so many things wrong with the code you
posted, it's hard to know where to start. So, I guess I will just start
at the top.
Please note that you may find this will be an iterative process. Many of
the problems are not directly related to how the basic functionality you
want should be architected, though there may be hints there. I'll try to
put some sort of useful pseudocode at the end as a way of suggesting the
correct architecture.
So, on with the comments...
The second method that I adopted looks like this:
private void btnConnect_Click(object sender, EventArgs e)
{
IPAddr = tbxIPAddress.Text;
This is not great:
//Validate IP Address
if (!IsValidIPAddress(IPAddr))
{
MessageBox.Show("Invalid IP Address", "Input Error",
MessageBoxButtons.OK, MessageBoxIcon.Error);
return;
}
There's no need to validate the IP address. When you pass it to the
TcpClient, it will validate it for you. If the string cannot be resolved,
you'll get an exception. Handle the exception then, converting to plain
English if appropriate (you'll get socket error that relates to the
invalid IP address. If your code concerns itself with the specifics of
how an IP address looks, you lose out on functionality like using IPv6 and
named hosts (things that TcpClient should handle automatically for you if
you let it).
This part is awful:
//Storing the IP Address in file for next time
// FileStream file = new FileStream("C:\\IPAddress.txt",
FileMode.Create, FileAccess.Write);
// StreamWriter sw = new StreamWriter(file);
// sw.Write(IPAddr);
// sw.Close();
You should not be saving a file to the root directory of the C: drive, and
you should not really be using a file like this anyway. Just create a
"User" scope setting in your project to store the IP address. You can
access it using Properties.Settings.Default.IPAddress (assuming you call
it "IPAddress"). After setting it, you need to call
Properties.Settings.Default.Save(), to make sure it's stored in the
user.config file. You can retrieve it at any time; it will persist in the
user.config file after you call Save() and will be available when you run
the application next time (and any time after that).
//Connecting to the Network Recorder
this.lblConnStatus.Text = "Trying to Establish
Connection...";
This part is awful:
Application.DoEvents();
System.Threading.Thread.Sleep(3000);
There is no need to ever call DoEvents() in properly architected code.
There is certainly no need here, as you have not done anything at this
point that would block the code for any significant period of time before
calling DoEvents(). It's unlikely the call to DoEvents() will do anything
significant.
As for the call to Sleep()...what are you thinking here? You are just
arbitrarily stopping program execution for 3 seconds. What's the point of
that? What are you waiting for, and why does it seem reasonable to make
the user wait 3 seconds?
This part is not great:
server = new TcpClient(IPAddr, port);
You want to use TcpClient asynchronously. So use BeginConnect() as well
as BeginReceive(). Not only does it avoid the UI updating issues, it
makes it a lot easier to address the retry-on-failure design you're trying
to accomplish here.
state = new StateObject();
state.workSocket = server.Client;
This part is awful:
server.Client.BeginReceive(state.buffer, 0,
StateObject.BufferSize, 0, new AsyncCallback((new
RecorderMonitor()).OnReceive), state);
You are creating a new instance of your form class here. Bad! You never
show it, which is why later on you get the exception you mentioned in a
follow-up to this post (using the window handle before creating it). But
beyond that, I see nothing in your posts that suggest you want a new
instance of the form. You want to use the existing form, the one that the
user has been interacting with already. So use that one: "new
AsyncCallback(OnReceive)".
This part is not great:
status = true;
this.Invoke(this.m_DelegateSetAppParam);
}
The "status" flag is almost certainly superfluous. There are likely
better ways to accomplish whatever it is you think you're doing there.
But more significantly, the call to Invoke() is completely superfluous.
You are handling the button click event. You are guaranteed to be in the
form's owning thread. There is absolutely no need to use Invoke() in this
context. Just call the delegate directly: "this.m_DelegateSetAppParam();".
That, of course, assumes that there's a real point to that delegate. I'll
take as granted that there is.
This part is not great:
catch (SocketException)
{
status = false;
RetryConnect(this);
//return;
}
In particular, you should at least check the socker error in the
exception. Not all exceptions should lead you to think that retrying the
connection attempt would actually work. There are certain exceptions for
which you can just give up now.
If you use BeginConnect(), then there is no need for a "retry" function.
The callback provided to BeginConnect() can simply reissue the
BeginConnect() when appropriate, specifying itself as the callback.
Note that this retry function has the same things wrong with as the button
click handler: calling DoEvents(), arbitrarily sleeping for 5 seconds,
creating a new RecorderMonitor() instance, etc. In addition...
RetryConnect functions looks like this
public void RetryConnect(RecorderMonitor m_Form)
{
//this.lblConnStatus.Text = "Retrying to Establish
Connection...";
Application.DoEvents();
System.Threading.Thread.Sleep(5000);
try
{
server = new TcpClient(IPAddr, port);
state = new StateObject();
state.workSocket = server.Client;
server.Client.BeginReceive(state.buffer, 0,
StateObject.BufferSize, 0, new AsyncCallback((new
RecorderMonitor()).OnReceive), state);
}
catch (SocketException)
{
status = false;
This is not great:
if (this.lblConnStatus.InvokeRequired)
{
RetryConnectCallBack d = new
RetryConnectCallBack(RetryConnect);
this.Invoke(d, new object[] { m_Form });
}
else
{
m_Form.lblConnStatus.Text = "Network Recorder is
down";
// m_Form.btnConnect.BackColor =
System.Drawing.Color.Red;
Application.DoEvents();
}
Some things wrong with the above block:
* If you do the Begin/EndXXX stuff correctly, there is no need to
check InvokeRequired. In your callbacks, you will be guaranteed that you
need to call Invoke() and in the UI code you will be guaranteed that you
don't need to. That's one advantage (of many) with respect to using the
async pattern correctly.
* The code has this very peculiar trait that it only treats an
exception as fatal ("Network Recorder is down") once it's been run on the
main thread. So, if you call it while the initial connect is attempted,a
failure here is fatal. But if you call it from your asychronous
BeginReceive() callback, a failure here results in a retry. That retry
gets put on the main thread, and so a subsequent failure there _is_
fatal. While I don't see any obvious harm in this design, it's overly
complicated. Overly complicated often leads to hard-to-find bugs. It's
just not a good idea.
* You call DoEvents(). For the same reason this is bad elsewhere,
it's bad here.
return;
}
In the btnconnect event... I just tryt o connect to the remote
application
only once and repeatedly try connecting to it unless the connection is
established.
I don't see a repeated attempt to connect. As near as I can tell, you
make one retry attempt after the initial failure, and then give up. This
may or may not be correct, depending on what your intent was.
I am not having any trouble in doing that. The trouble comes
when I am connected to the server and start receiving the Data using the
Begin Receive which calls the following function:
As with the previous code, there are problems. More comments...
public void OnReceive(IAsyncResult ar)
{
String content = String.Empty;
// Retrieve the state object and the handler socket
// from the asynchronous state object.
StateObject state = (StateObject)ar.AsyncState;
Socket handler = state.workSocket;
int bytesRead;
This is not great:
Don't bother checking for connected. If the socket has become
disconnected, you'll find out when you call EndReceive() and get an
exception. This if() statement is superfluous, and complicates your flow
control in the function (in particular, you have no retry logic if the
socket is not connected).
{
// Read data from the client socket.
try
{
bytesRead = handler.EndReceive(ar);
if (bytesRead > 0)
{
// There might be more data, so store the data
received so far.
state.sb.Remove(0, state.sb.Length);
state.sb.Append(Encoding.ASCII.GetString(
state.buffer, 0, bytesRead));
This is awful:
// Display Text in Rich Text Box
// content = state.sb.ToString();
You are guaranteed at this point to be on a different thread from the main
UI thread. You need Invoke() or BeginInvoke() if you are going to set
something in the main form directly or otherwise run some code on the main
UI thread (which is not a bad way to synchronize access to shared data
structures in a form, even if it doesn't require updating something in the
displayed form itself). Alternatively, you need to use some
multi-thread-synchronized data object to pass data from your i/o worker
thread to whatever thread in your application will process it.
// Console.WriteLine(content);
handler.BeginReceive(state.buffer, 0,
StateObject.BufferSize, 0,
new AsyncCallback(OnReceive), state);
}
}
In the above code, it repeatedly keeps calling BeginReceive to ensure it
is
continuously r eceiving data from server... But now I need to handle a
situation where if the server goes down.. the catch block tries to retry
connecting as it was doing in the previous case. Here whenever I call
RetryConnect function, it says invalid operationexception after executing
that function. Basicallu its not able to update the UI ... may be bcause
in
this case RetryConnect belongs to a different thread.
The exception isn't because of a thread issue. It's because you createda
new instance of your form and never showed it, when initializing the
BeginReceive() callback.
How should update the
UI just like I was doing before from btnConnect event.. in this case. I
even
tried to declare RetryConnect as a Delegate but it doesnt work.
Here's the basics:
Main thread:
-- button click: create TcpClient, call
TcpClient.BeginConnect(string, Int32)
Worker callbacks (which run on threads that are automatically managed
for you):
-- connect callback: call EndConnect(), if successful then call
BeginReceive(), if not then use Invoke() to update form's status text and
call BeginConnect(string, Int32) again if appropriate (don't do this if
you want the retrying to stop)
-- receive callback: call EndReceive(), if successful then use
Invoke() to update UI with received data for status and call
BeginReceive() again, if failed then use Invoke() to update UI with status
and call BeginConnect(string, Int32) to try again (and as above, only if
you want to retry)
If the callback methods are instance methods in your form class (as they
appear to be in your posted code), then you always have access to your
form instance members from the callback methods. Do _not_ create a new
instance of your form when providing the callback methods.
Hope that helps.
Pete
