Objects Persist Across Threads - Please Help.

[pbd22]

Hi.

I have a telnet application (which many of you have already helped me
with, thank you).
I am now running into a bit of a serious design problem.

It seems that each new thread (telnet connection) is "connected" to
the pre-existing thread.
So, If I connect and log on as user1.
And, user1 adds value1, value2, and value3 to some variables.
And then user2 connects.
User2 sees the values set by the previously log'd in user.
And, what is worse, the previous user no longer has acess to his
telnet session.
It has been hijacked by the subsequent user.
Should a third user log on, the second user will lose control and so
on.
I am thinking this may piss people off .

Can this be addressed conceptually or should I dump my code here (I
wasn't really sure what to post code-wise).

Thanks again.
Peter

 

[Peter Duniho]

Hi.

I have a telnet application (which many of you have already helped me
with, thank you).
I am now running into a bit of a serious design problem.

It seems that each new thread (telnet connection) is "connected" to
the pre-existing thread.
[...]

Can this be addressed conceptually or should I dump my code here (I
wasn't really sure what to post code-wise).

I don't think there's any point posting code. If you do, you would not
post all of it. You would reduce the code to the bare minimum required to
reproduce the problem and then post that reduced version.

That said, it sounds clear that you are using a single object instance to
handle all of the data for all of the users. Obviously, this is wrong.

You should create a data structure that will contain whatever data is
unique for each user. Then for each user connected, you'll create an
instance of that data structure and reference it whenever you are
processing a particular user.

For what it's worth, you should not be creating a new thread for each
connected user. This will work okay for relatively small numbers of
users, but it won't scale at all as the number of connections increases,
and the code will work better with a different mechanism even for smaller
users.

I prefer the asynchronous APIs, involving the methods that start with
"Begin" and "End". With these methods, you pass an object reference as a
"state" value to the "Begin" method. This value is then put into the
IAsyncResult instance for future reference, as the IAsyncResult.AsyncState
property. So, for example, if you use the callback technique (IMHO the
way that is usually best), you can retrieve your object instance from the
IAsyncResult passed to your callback method. Just cast it back to your
own class, and you've got your per-user data for the operation (likely
including the Socket reference as part of this data structure, which you'd
need in the callback in order to call the "End" method, and a buffer
reference, so that you can retrieve received data once an operation has
completed, to name a couple of things that commonly go into a data
structure like this).

Pete

 

[pbd22]

Thanks Pete,

I have seen async code examples but, before I follow this
new path, I want to make sure that my approach can't be
tweaked a bit to fix the design issue.

You should create a data structure that will contain whatever data is
unique for each user. Then for each user connected, you'll create an
instance of that data structure and reference it whenever you are
processing a particular user.

I thought that was what I was doing with the following code:

protected override void OnStart(string[] args)
{
this.timer1.Enabled = true;
new Thread(new ThreadStart(Listen)).Start();
}

public void Listen()
{

try
{
Queue unsyncq = new Queue();
connectionQueue = Queue.Synchronized(unsyncq);
}

....

Doesn't the Queue handle each connection?

Thanks again.

 

[Peter Duniho]

[...]
I thought that was what I was doing with the following code:

protected override void OnStart(string[] args)
{
this.timer1.Enabled = true;
new Thread(new ThreadStart(Listen)).Start();
}

public void Listen()
{

try
{
Queue unsyncq = new Queue();
connectionQueue = Queue.Synchronized(unsyncq);
}

...

Doesn't the Queue handle each connection?

Obviously not.

You have not posted a concise-but-complete sample of code that reliably
reproduces the problem. In fact, your sample of code has precious little
information regarding what's actually going on. For example, just because
you've created an instance of the Queue class, that doesn't mean you're
using it in a useful way. But the fact that your client connections
interfere with each other pretty much guarantees that whatever code you've
got, it's not keeping the data for each client separate.

Switching the i/o paradigm for your code is not in fact a panacea.
Certainly you should be able to fix the problem without doing so, and
doing so is not going to guarantee that the problem will go away. In
fact, as compared to dedicating a thread to the connection, the design
change is going to be similar either way: you need a data structure that
is allocated for each connection and which maintains all of the state that
will be used in the processing of that connection.

Pete

 

[pbd22]

OK Pete, thanks.

"But the fact that your client connections
interfere with each other pretty much guarantees that whatever code
you've
got, it's not keeping the data for each client separate. "

I agree. I messed up somewhere. I think its in my connection handler
but
I am having a hard time isolating where. Below is the connection
handler
with the condition logic taken out (mostly). Does this reveal any
errors?

Thank a ton.
Peter

This try block is still in the Listen() method:

try
{

TcpListener listener = new TcpListener(_telnetServer,
_telnetPort);

listener.Start();

while( true)
{

_telnetSocket = listener.AcceptTcpClient();
connectionQueue.Enqueue(_telnetSocket);

Thread workingthread = new Thread(new
ThreadStart(TheConnectionHandler));
workingthread.Start();

}

}


And here is the TheConnectionHandler that handles each new connection
(it is a lot of code.
I tried to distill it a bit):

public void TheConnectionHandler()
{

_telnetSocket = (TcpClient)connectionQueue.Dequeue();
_clientStream = _telnetSocket.GetStream();


SendPrompt("Login: ");

while (true)
{

_bytesRead = 0;

try
{

//blocks until a client sends a message
_bytesRead = _clientStream.Read(message,
0,4096); //message

}
catch
{
//a socket error has occured
break;
}
if (_bytesRead == 0)
{
break;
}

statusMessage += ASCII.GetString(message, 0,
_bytesRead);


int ichEOL = statusMessage.IndexOf(ENDOFLINE);

// If there is an EOL character, then process the
current line
//if (ichEOL >= 0)
//{
if (statusMessage.LastIndexOf(ENDOFLINE) > 0)
{


strLine = statusMessage.Replace(ENDOFLINE,
"");


if (ichEOL < statusMessage.Length)
{
//statusMessage =
statusMessage.Substring(ichEOL);
statusMessage =
statusMessage.Substring(ichEOL).Replace(ENDOFLINE, "");
}
else
{
statusMessage = "";
}

if (loginState != LoginState.Connected)
{
Login(strLine);
}

if (loginState == LoginState.Connected)
{
// do something
}

}

}

_telnetSocket.Close();

}

 

[Peter Duniho]

OK Pete, thanks.

"But the fact that your client connections
interfere with each other pretty much guarantees that whatever code
you've
got, it's not keeping the data for each client separate. "

I agree. I messed up somewhere. I think its in my connection handler but
I am having a hard time isolating where. Below is the connection handler
with the condition logic taken out (mostly). Does this reveal any
errors?

Well, I don't see the point in the use of the queue. Why not just pass
the socket reference to the constructor of TheConnectionHandler? The
queue isn't gaining you any advantage here.

But I also don't see the error, since you don't seem to have posted the
code that is actually defining where your data is stored.

I suspect that the "statusMessage" variable is a static variable, or
otherwise not an instance variable of TheConnectionHandler. That would
explain to some degree the behavior you're describing. But it's not
possible to say from the code you posted.

Basically, though, _all_ of the data relevant to a specific connection
should be an _instance_ member of TheConnectionHandler. This will ensure
that the code in TheConnectionHandler that is handling the i/o for a given
connection is always using only the data for that connection and not
interfering with some other connection.

Pete

 

[pbd22]

Thanks Peter.

I think this is probably the problem. Here are the declarations:


string statusMessage = string.Empty;
string result = string.Empty;
string strLoginID = string.Empty;
string programName = string.Empty;
string fileName = string.Empty;
string address = string.Empty;
DateTime startDate;
DateTime endDate;
DateTime startTime;
DateTime endTime;
string strLine = null;

They get populated at various points in the programs life-cycle. The
statusMessage
string reades the user's bytestream and handles that data.

To be honest, I haven't really written the type of class you are
talking about but after
googling I am wondering if following the below convention is correct:

public class DataInstance
{
private string statusMessage = "";

public DataInstance(string str)
{
statusMessage = str;
}

public string StatusMessage
{
get
{
return statusMessage;
}
}
}


should I be defining all the variables (above) that are particular to
the TheConnectionHandler as
methods within this type of class structure?

Thanks.
Peter

 

[Peter Duniho]

Thanks Peter.

I think this is probably the problem. Here are the declarations:

Where are those declared? All you've provided there is the type, field
name, and initializer. That doesn't provide _any_ information as to
scope, and this question is entirely about scope.

Any data that you want to be unique for each user, it needs to be in a
class that is unique for each user. That _appears_ to be
TheConnectionHandler, but really...you keep _not_ posting complete code
samples, or even code samples that have enough context to make any useful
inferences.

No one can answer your questions if you don't provide enough information
for the questions to be answered.

[...]
should I be defining all the variables (above) that are particular to
the TheConnectionHandler as methods within this type of class structure?

I believe you mean "as properties", as that's what the code sample you
found is doing. And yes, I think encapsulating fields in properties is
useful. But it has nothing to do with the question at hand.

What _does_ have something to do with the question at hand is where those
"variables (above)" are declared. Those that are unique for each user,
they need to be stored in a class instance that is unique for each user.

Pete

 

[pbd22]

Thanks Peter.

All the code I am dealing with right now is in a single class inside a
Windows Service project.
All declarations happen at the head of the file (which is the root of
my problem, i am sure).

""""Any data that you want to be unique for each user, it needs to be
in a
class that is unique for each user. That _appears_ to be
TheConnectionHandler,""""

TheConnectionHandler is not a class. It is a method inside my main
class file that is called
to handle each incoming connection using:

while( true)
{

_telnetSocket = listener.AcceptTcpClient();
connectionQueue.Enqueue(_telnetSocket);

Thread workingthread = new Thread(new
ThreadStart(TheConnectionHandler));
workingthread.Start();

}

TheConnectionHandler users an ENUM block at the top of the class to
get various states. Inside
this method there are a bunch of conditions used to assign user input
to variables assigned at the
declarations section of the class (below).

if (recordState ==
RecordState.ProgramName)
{
try
{
programName = strLine;
}

Most of the user-specific variables are defined inside
TheConnectionHandler in this way.
For what it is worth, the strLine variable (declared as shown below)
is the statusMessage
input string with all the telnet specific characters parsed out (hello
\r\n = hello).

I am guessing that inside the TheConnectionHandler method I should be
instantiating the
class of user-specific properties that you are talking about. I hope I
have given you enough
information about what my class structure looks like and how I am
currently handling data.
I would appreciate it if you could give me some sense as to how to
create a class instance
that is "unique for each user". I am new to this.

The below variables have been declared at the head of my class file.
Most of these variables are unique to each telnet connection (the ones
that may
appear not relevant, i left in as i wasn't sure). They are as follows:



public partial class MyWindowsService: ServiceBase
{

#region Variables


//Misc Global variables
private int bytes = 0;
private int _statusCode = 0;
private int _bytesRead;
private byte[] buffer = new byte[BUFFER_SIZE];
private byte[] message = new byte[MESSAGE_SIZE];
private TcpClient _telnetSocket = null;
private Thread listenerThread = null;
private Queue connectionQueue = null;
private NetworkStream _clientStream = null;
private LoginState loginState;
private RecordState recordState;
#endregion

# region sdk declarations
private string SDKAddress = string.Empty;
private HttpWebRequest _telnetHttpWebRequest;
private Uri _telnetUri;
private string err = string.Empty;
private string name = string.Empty;
static string sessionId = null;
static string uniqueId = null;
static string ingestTitle = null;
static uint scheduleId;
SDKUserProfile userProfile = null;
#endregion

# region other variables
string statusMessage = string.Empty;
string result = string.Empty;
string strLoginID = string.Empty;
string programName = string.Empty;
string fileName = string.Empty;
string address = string.Empty;
DateTime startDate;
DateTime endDate;
DateTime startTime;
DateTime endTime;
string strLine = null;
# endregion

public MyWindowsService()
{

 

[Peter Duniho]

Thanks Peter.

All the code I am dealing with right now is in a single class inside a
Windows Service project.
All declarations happen at the head of the file (which is the root of
my problem, i am sure).

Ah. Yes, this is a serious problem. You need to fix your code so that
you have a class that is specific to the management of a single
connection. Within that class will be any fields that are unique for each
connection, as well as any code that deals with managing a single
connection (for example, now that I realize that "TheConnectionHandler" is
a method, I would suggest that's probably a method that should go into the
per-connection class).

Then, when you create a new connection, you'll instantiate an instance of
this per-connection class, thereby ensuring that each connected client has
its own copy of any data that is unique for each connected client. That
way, what happens in one client won't affect what happens in another.

I also see now why you were using a Queue -- you didn't have an
appropriate place to put the TcpClient reference since you didn't have a
per-connection class. It still wasn't the best way to do that (you could
have used the ParameterizedThreadStart delegate for your thread), but once
you've got a per-connection class you can just pass that TcpClient
reference to the constructor of that class rather than messing around with
the queue.

Pete

 

[pbd22]

OK, thanks.

I am going to follow the below "format" for my per-connection class as
I have not
done this before and need some sort of example. Let me know if this
isn't a good
template. Otherwise, I'll see how it goes and thanks again.

using System;
namespace MyObjects
{
public class UserInfo
{
//Default constructor
public UserInfo()
{
}

//Additional Constructor
public UserInfo(object fName, object lName, object pNumber,
object currentVisits)
{
m_firstName = fName;
m_lastName = lName;
m_phoneNumber = pNumber;
m_visits = currentVisits;
}


//Specify private properties
string m_firstName;
string m_lastName;
string m_phoneNumber;
int m_visits;


//Specify public methods
public void IncrementVisits()
{
m_visits += 1;
}


//Specify public properties
public string FirstName {
get { return m_firstName; }

set { m_firstname = value; }
}

public string LastName {
get { return m_LastName; }

set { m_LastName = value; }
}

public string PhoneNumber {
get { return m_PhoneNumber; }

set { m_PhoneNumber = value; }
}

public int Visits {
get { return m_Visits; }

set {
if (value < 0) {
m_visits = 0;
}
else {
m_Visits = value;
}
}
}
}
}

 

[pbd22]

OK, thanks.

I am going to follow the below "format" for my per-connection class as
I have not
done this before and need some sort of example. Let me know if this
isn't a good
template. Otherwise, I'll see how it goes and thanks again.

using System;
namespace MyObjects
{
public class UserInfo
{
//Default constructor
public UserInfo()
{
}

//Additional Constructor
public UserInfo(object fName, object lName, object pNumber,
object currentVisits)
{
m_firstName = fName;
m_lastName = lName;
m_phoneNumber = pNumber;
m_visits = currentVisits;
}


//Specify private properties
string m_firstName;
string m_lastName;
string m_phoneNumber;
int m_visits;


//Specify public methods
public void IncrementVisits()
{
m_visits += 1;
}


//Specify public properties
public string FirstName {
get { return m_firstName; }

set { m_firstname = value; }
}

public string LastName {
get { return m_LastName; }

set { m_LastName = value; }
}

public string PhoneNumber {
get { return m_PhoneNumber; }

set { m_PhoneNumber = value; }
}

public int Visits {
get { return m_Visits; }

set {
if (value < 0) {
m_visits = 0;
}
else {
m_Visits = value;
}
}
}
}
}

 

[Peter Duniho]

OK, thanks.

I am going to follow the below "format" for my per-connection class as I
have not
done this before and need some sort of example. Let me know if this
isn't a good
template. Otherwise, I'll see how it goes and thanks again.

Well, I don't see anything wrong with that pattern per se. But it doesn't
seem to have anything to do with a network i/o class. You also would not
necessarily want to make every property public. After all, there are sure
to be a number of fields within the per-connection class that are for use
only within that class. For those, you may not need to wrap them in
properties at all, but for sure you wouldn't want to make public any
properties related to them.

Frankly, the class you posted seems more to just be an example of "how to
write a class". If you are at that stage in your level of experience with
C# and object-oriented programming more generally, then I respectfully
suggest that writing a telnet server is probably not the best place to
start. Network i/o code isn't IMHO _extremely_ difficult to write, but it
isn't trivial either. Trying to figure out a network i/o implementation
at the same time that you're trying to just learn how to write in an
object-oriented way is a classic set-up for information overload. You may
wind up not being able to do a good job at either, combining the two like
that.

It's just a thought. I realize that there's always more to a situation
than is immediately apparent, and I especially am familiar with the
problem of a person being a position where they don't really have a choice
regarding the thing they are trying to implement. Sometimes there's a job
to get done, and you have to do it no matter how ill-prepared you are for
it. But if you have any leeway at all to separate this problem into
smaller pieces so that you can learn one piece without being distracted by
any other, I _strongly_ recommend you do so. You'll be much better served
in the long run with that approach.

Pete

 

[pbd22]

Thanks Pete,

Sometimes there's a job to get done, and you have to do it

That is pretty much it in a nutshell. If I don't produce, somebody
else
will - plain and simple. I have various programming strengths but,
this
is my first telnet implementation so, any hands-on examples or help
here would be very much appreciated. Unfortunately, the deadline is
tuesday evening so, I really don't have the luxury of time or
"learning
exercizes" right now. Its the reality of work and life as you are
well
aware.

Thanks again.
Peter

 

[pbd22]

""""""""for example, now that I realize that "TheConnectionHandler"
is
a method, I would suggest that's probably a method that should go into
the
per-connection class""""""""""

So, how, exactly, would this work. If TheConnectionHandler is a method
in the
per-connection class, and the per-connection class name is
UserConnection,
then the class instantion should look like this(?):


Thread workingthread = new Thread(new ThreadStart(new
UserConnection));
workingthread.Start();

And, now I have my UserConnection class set up to accept connection-
specific
values. But, how exacly do you envision the TheConnectionHandler to be
"inside"
the UserConnection class??? It seems to me that the UserConnection
values
should be get/set inside TheConnectionHandler. Please explain - code
examples
as illustration helps if need be. Thanks.

namespace VBPortListener
{
public class UserConnection
{

private string m_programName = string.Empty;
private string m_fileName = string.Empty;
private string m_address = string.Empty;
private DateTime m_startDate = DateTime.MinValue;
private DateTime m_endDate = DateTime.MinValue;
private DateTime m_startTime = DateTime.MinValue;
private DateTime m_endTime = DateTime.MinValue;

// Declare a Name property of type string:
public string ProgramName
{
get
{
return m_programName;
}
set
{
m_programName = value;
}
}

// Declare an Age property of type int:
public string FileName
{
get
{
return m_fileName;
}
set
{
m_fileName = value;
}
}

// Declare a Name property of type string:
public DateTime StartDate
{
get
{
return m_startDate;
}
set
{
m_startDate = value;
}
}

// Declare a Name property of type string:
public DateTime EndDate
{
get
{
return m_endDate;
}
set
{
m_endDate = value;
}
}

// Declare a Name property of type string:
public DateTime StartTime
{
get
{
return m_startTime;
}
set
{
m_startTime = value;
}
}

// Declare a Name property of type string:
public DateTime EndTime
{
get
{
return m_endTime;
}
set
{
m_endTime = value;
}
}


}
}

 

[Peter Duniho]

So, how, exactly, would this work. If TheConnectionHandler is a method
in the
per-connection class, and the per-connection class name is
UserConnection,
then the class instantion should look like this(?):


Thread workingthread = new Thread(new ThreadStart(new
UserConnection));
workingthread.Start();

No, you have to use a method to construct the ThreadStart delegate. So
assuming you move the method "TheConnectionHandler" into the class
"UserConnection", you'll have something like this:

UserConnection uc = new UserConnection(listener.AcceptTcpClient());

Thread workingthread = new Thread(uc.TheConnectionHandler);

(Note that you don't really need to explicitly construct the ThreadStart
delegate...passing a compatible method will cause the compiler to
automatically do that for you. Note also the example of how you'd use the
constructor to pass the TcpClient reference directly to the UserConnection
instance).

For many applications, they will want to actually manage some sort of data
structure that contains the collection of UserConnection instances. In
that case, you would either want the UserConnection constructor or the
method "TheConnectionHandler" to add the class instance itself to that
collection, or you would want to use the "uc" variable to add the
reference to a collection where you create the instance.

And, now I have my UserConnection class set up to accept
connection-specific
values. But, how exacly do you envision the TheConnectionHandler to be
"inside"
the UserConnection class???

The same way any other method is "inside" a class. You put it there. As
a member of that class.

It seems to me that the UserConnection values
should be get/set inside TheConnectionHandler. Please explain - code
examples
as illustration helps if need be. Thanks.

With the method inside the UserConnection class, it has the same ready
access to the fields as it had previously when the method was in your
MyWindowsService class and the fields were also in that class. Nothing's
changed except where those things are. You shouldn't need a code example,
because the code itself shouldn't need to change. Just move everything
that needs to move.

Now, the method _may_ need to get access to things that remain in the
original MyWindowsService class, and of course _that_ will change if you
move the method. But in that case, there's nothing special about it. You
just need for the UserConnection class to have a way to get a reference to
the MyWindowsService class and access those items (preferably wrapping the
fields in properties for public access). You can either pass that
reference to the constructor for the UserConnection class, or as is likely
to be suitable for a top-level class managing a service, make the class a
singleton or possibly even a static class (if you know that there's no
reason to ever inherit it), so that it's accessible statically.

Pete