🚀 go-pugleaf

"James" <jb9@btinternet.com> wrote in message
news:417ce71f$1@newsgroups.borland.com...

> Ok, I'm trying to allow both the OnExecute thread from TIdTCPServer,
> and another thread that I created, to have access to a client's AThread
> instance. The code below shows how I'm trying to do this, but I've
> taken a lot of stuff out (like try/catch blocks etc).

Your thoughts are close, but I would suggest an alternative implementation
instead.  You should move the critical section into the TClientInfo class.
That way, each connection has its own individual critical section.  Don't
share a single global critical section for all connections.  You will slow
down your entire code too much by doing that, especially if you are going to
have a lot of clients connected at a time.

    class TClientInfo
    {
    private:
        TCriticalSection* FCS;
    public:
        TIdPeerThread* Thread;
        AnsiString IP;
        AnsiString Name;

        TClientInfo(TIdPeerThread *AThread);
        ~TClientInfo();

        AnsiString Read();
        void Write(const AnsiString &Str);
    };

    TClientInfo::TClientInfo(TIdPeerThread *AThread)
    {
        FCS = new TCriticalSection;
        Thread = AThread;
        if( Thread )
        {
            IP = Thread->Connection->Socket->Binding->PeerIP;
            Name = GStack->WSGetHostByAddr(IP);
            // etc ..
        }
    }

    TClientInfo::~TClientInfo()
    {
        delete FCS;
    }

    AnsiString TClientInfo::Read()
    {
        AnsiString Result;
        FCS->Enter();
        try {
            Result = Thread->Connection->ReadLn("END", IdTimeoutInfinite,
100);
        }
        __finally {
            FCS->Leave();
        }
        return Result;
    }

    void TClientInfo::Write(const AnsiString &Str)
    {
        FCS->Enter();
        try {
            Thread->Connection->Write(Str);
        }
        __finally {
            FCS->Leave();
        }
    }

    void __fastcall TForm1::TCPServerConnect(TIdPeerThread *AThread)
    {
        TClientInfo *info = new TClientInfo(AThread);
        AThread->Data = (TObject*) info;
        ListOfClients->Add(info);
    }

    void __fastcall TForm1::TCPServerDisconnect(TIdPeerThread *AThread)
    {
        TClientInfo *info = (TClientInfo*) AThread->Data;
        AThread->Data = NULL;

        ListOfClients->Remove(info);
        delete info;
    }

    void __fastcall TForm1::TCPServerExecute(TIdPeerThread *AThread)
    {
        TClientInfo *info = (TClientInfo*) AThread->Data;

        AnsiString sRaw = info->Read();
        AnsiString sCmd = Decrypt(sRaw);

        info->Write("something");
    }

    void __fastcall ControlThread::Execute()
    {
        Sleep(5000);
        for(int iter = 0; iter < ListOfClients->Count; ++iter)
        {
            TClientInfo* Client = (TClientInfo*) ListOfClients->Items[iter];
            Client->Write("Ping!");
        }
    }

> I also realise I could use a TThreadList to store the TClientInfos instead
> of TList.

Yes, you should be doing that, since your list is being accessed by multiple
threads at a time.  Also, why are you storing your own list to begin with?
TIdTCPServer already contains a TThreadList of all the currently active
threads.

> From reading previous posts, the problem is that AThread is not
> threadsafe, which means that TCPServerExecute can't sit there
> waiting for the string "END" while ControlThread::Execute is using
> the AThread to send "Ping!"

Not true.  One thread can read from the socket while another thread writes
to the socket.  That is perfectly fine.  What is not safe is having the two
threads reading from the socket at the same time, or writing to the socket
at the same time.  Those are the situations that you need to provide
synchronization for.

The simpliest way to do that is to not have multiple threads both reading or
both writing in the first place.  Make it so OnExecute does all of the
reading and nothing else, and then have a separate thread that contains a
queue of outgoing data and does all of the writing from the queue in a
serialized manner and nothing else.  Then you don't have to protect the
socket with a critical section at all.  That model may not fit into your
current design, though.  So it is your decision which way you want to
approach the issue.

Your current example is certainly not safe, though.


Gambit