Hi Remy, thanks for the advice, left my thoughts below!
(BTW this is quite a long one so you might want to leave it for a while
until you got some spare time)
>
>> Users: Array of TUser;
>
> That array is not thread-safe. You should use a TThreadList instead.
I was accessing it through a CriticalSection so that only one thread at a
time could access the array..
>> If Users[I].Connection.Thread = AThread then
>
> That is exactly what I'm telling you NOT to use. If you must find a
> TUser from a TIdPeerThread pointer, then I suggest you put the TUser
> pointer into the TIdPeerThread's Data property. Then you don't have
> to look up anything at all.
Ok, let me digest this bit and get back to you on it later...
Meanwhile...
>> //When writing to the client...
>>
> TIdPeerThread(Users[I].Connection.Thread).Connection.WriteLn(TheMessag
> e);
>
> Make sure that is thread-safe. Writing to the Connection from a
> different thread than the one that is managing the client can cause
> packets to overlap if you are not careful. You should wrap the
> writing in a critical section to prevent multiple threads writing to
> the socket at the same time.
Yes, all the threads go straight into a CriticalSection as soon as the
OnExecute event is fired. They then leave the CS when they are done with
their 'doings'. Is it safe to write from one thread to another like that or
should I create a seperate thread (or use the main with timer) to do the
writing?
>> I'd seen the above method of addressing the AThread used in one of
> the
>> old Indy 8 demo's by: Jeremy Darling (though admittedly he also used
>> Threads.Locklist and client objects which doesn't work for me
> because
>> I need to track ALL online AND offline users in the same place - EG
>> an array.
>
> There is nothing preventing you from doing that with a TThreadList.
Well there is... a) my lack of understanding of the TThreadList and...
b) I thought a TThreadList had to have a connection to the server?
As mentioned before, I wanted to keep the offline users in the same place
(array) aswell as the online users. Can I actually store offline client
details in a TThreadList if they do not have an active connection? - If so,
how?
> Your array/list/whatever must be thread-safe, or else you risk
> screwing up your data, i not crashing the code altogether.
Yes, as mentioned previously above, all my threads are entering a
CriticalSection immediately. Whilst I didn't actually include that in the
code I posted yesterday (it was a simplifed version of my code) my code does
make use a CS.
>> The IdTCPServer threadlist of course only contains those active
>> connections which is why I have avoided using it.
>
> It only stores the thread pointers. There is nothing stopping you
> from storing your user pointers in your own list as well.
Yes, but if I store the offline users in my own list, I end up with 2 lists-
the TThreadList and TMyList. Something I wanted to avoid... Unless your
saying I could simply store ALL in TMyList and ignore the TThreadList?
>> Yes, but why is that better than simply using the method above to
>> match the AThread with a TIdPeerThread ?
>
> Because you are matching up your users with a pointer that is dynamic
> and can go away at any time - even while you are looking for it. It
> is not a good way to keep track of your users.
Ah... a breakthrough... I think!
You see, this is where I'm getting confused. If the pointer can go away at
any time, then how can the AThread.Data object still be correctly assigned?
What I'm saying is, If I can't use AThread = Users[I].Thread then how is
using AThread.Data safer?
Surely if the AThread is dynamically reassinged, it would take the .Data
with it as it is a property of the AThread???
Sorry for sounding dumb but it seems kinda logical that way.. which is why
I'm having trouble grasping the concept of using .Data instead of AThread.
>> I still don't really see why I must use the data object
>
> You've already said why - because you need to keep track of offline
> users. That in itself requires you to have a separate data object for
> your user data.
Yes, but thats why I was using a dynamic array - each user has an indexed
record.
In reality, whats the difference between using a dynamic array to store
users details and using a client object? - They both store strings and other
details which is why I'm not clearly seeing the benefits of what your
saying. Sorry I don't mean to be dumb, I'm just not very experienced with
the concept you are trying to demonstrate to me.
> It just happens when a user is online, it can be
> associated with a thread pointer as well.
Yes, that was soemthing like I was trying to do - Associate a user with a
thread pointer. Whilst admittedly also trying to identify the user from the
thread at the same time..
>> I'm not trying to associate the thread with an object
>
> Yes, you are.
>
>> more like keep a record of which user is associated with which
> thread.
>
> In other words, to associate a thread with a [user] object.
Well, yes, but then above you also said: " It just happens when a user is
online, it can be associated with a thread pointer as well.". Not trying to
be clever or funny here but that from my POV seems to be suggesting the same
thing?!? - with the exception of trying to identify the user from the
thread.
>> Well I'm basically just trying to keep all my users (both on and
> offline)
>> in one place - A dynamic array.
>
> Try this instead:
>
>> Not really keen on playing with AThread.Data property to be honest!
>
> You have to, for exactly the reason you just said - it is quick and
> makes the code easier to use.
RIGHT... think I might actually be seeing what your saying here now!!!!!
<gets excited>
So, does the TUser object stay unique to each thread? - EG if it's created
in Thread1, then the TUser stays within THAT thread and isn't accessed by
all threads?... right?...
That being the case I can see the advantages. I forgot the threads share the
same code but act individually (Ok, I feel extremely stupid now but could
you just confirm thats the case so I know what I'm really doing?!). Sorry
top to this part of the message has been a waste now I got to this part and
found a light bulb above my head, but until I re-read this it didn't hit me
what I think your trying to say..
> type
> TUser = class
> public
> UserName,
> Password,
> Email,
> Session,
> Key,
> IP: String;
> Thread: TIdPeerThread;
> Lock: TCriticalSection;
> constructor Create;
> destructor Destroy; override;
> end;
>
> public
> Users: TThreadList;
As I understood it though, a TThreadlist was a property of the TIdTCPServer
and could only be used for active connections??
>
> constructor TUser.Create;
> begin
> inherited Create;
> Lock := TCriticalSection.Create;
> end;
So does each individual user need their own CS? - I was using one main CS
declared as public in the main unit and then each thread was making a call
to the CS to enter/leave - was I wrong to do this?
> procedure TForm1.IdTCPServer1Connect(AThread: TIdPeerThread);
> var
> //...
> List: TList;
> Client, User: TUser;
> I: Integer;
> begin
> // gather information from connecting client ...
>
> Client := nil;
> List := Users.LockList;
> try
> // check if an existing user is reconnecting...
> for I := 0 to List.Count-1 do
> begin
> User := TUser(List[I]);
Looking at this, could I simply take the User object/class directly from my
dynamic array? (Save me re-writing 1/2 the code)
EG: User := Users[I].UserDetails; ??
> procedure TForm1.IdTCPServer1Disconnect(AThread: TIdPeerThread);
> var
> List: TList;
> Client: TUser;
> begin
> Client := TUser(AThread.Data);
Ah... now this worries me, because above (at top of demo code) I made the
assumption that the Client object stayed with the thread all the way until
the client disconnected. Now it looks like I still have to find the client
from the AThread.Data each tiem I need to refer to it?
In that case, what advantage is there? - sorry I'm getting into a bit of a
muddle trying to get this clear in my head..
If AThread can be dynamically reassigned at any time, then how will Client
:= TUser(AThread.Data); work? - Surely the .Data would also be dynamically
reassigned with it? (Please tell me you understand what I'm trying to say?)
> AThread.Data := nil;
>
> if Client <> nil then
> begin
> Client.Lock.Enter;
> try
> Client.Thread := nil;
> finally
> Client.Lock.Leave;
> end;
> end;
>
> //...
> end;
>
> procedure TForm1.SendToUser(AUser: TUser; const Message: String);
> begin
> if (AUser = nil) or (Message = '') then Exit;
> // ensure the send is not occuring while the thread is being
> terminated
> AUser.Lock.Enter;
> try
> if AUser.Thread <> nil then
> AUser.Thread.Connection.WriteLn(Message);
> finally
> AUser.Lock.Leave;
> end;
> end;
Could the procedure above be called internally from any thread? - EG to send
from user1 thread to user2 thread? - I was doing something similar, using
CriticalSections but using my dynamic array..
> procedure GetThreadUserName(AThread: TIdPeerThread);
> begin
> if (AThread <> nil) and (AThread.Data <> nil) then
> Result := TUser(AThread.Data).UserName
> else
> Result := '';
> end;
Thanks for your help again!
