🚀 go-pugleaf

Another question:

7) The code I have shown is definitely not good:
After every 'ASK' button click, the reasponse is
slower and slower. Why?
(Maybe the thread is not terminated?)



-- 
Best regards,
Vladimir Stefanovic 

> (Maybe the thread is not terminated?)

I think I have found the 'bug'. I added:
   AThread->Connection->Disconnect();
...at the end of server code, and now the
speed is OK.

I thought that:
   IdTCPClient->Disconnect();
... will also close the thread (connection) on
server side.





-- 
Best regards,
Vladimir Stefanovic 

"Vladimir Stefanovic" <antivari@po.sbb.co.yu> wrote in message
news:434503e6$1@newsgroups.borland.com...

> How can I set the time in which:
>
>    IdTCPClient->Connect();
>
> ... will be waited to connect.

Since you appear to be using Indy 9, then Connect() has an ATimeout
parameter.

> Now, at my PC it's about 15 seconds, and I want to change that.

Simply change the value of the ATimeout parameter.

> If the separate thread is the only solution, can you please
> show me a piece of code.

TIdTCPClient already runs a thread internally to handle the timeout.

> Is it OK that client is responsible for both opening & closing
> the connection?

Yes.  That is typically how TCP is used in the first place.  Unless the
protocol dictates that the server should be the one who closes the
connection, then the client is usually the one who closes it.  This allows
the client to send multiple requests, and to decide when it is actually done
with the connection.

> Is the property TIdTCPServer::TerminateWaitTime, the
> time when server will anyway close the thread by itself
> (connection with client) which was not closed within the
> agreed time period?

TerminateWaitTime() only has effect when the server is being deactivated.
If there are any existing connections at the time the server is being
deactivated then they are are automatically closed and then the server waits
TerminateWaitTime number of milliseconds for all of the threads to be fully
terminated.  If there are still threads running after TerminateWaitTime
milliseconds have elapsed than an EIdTerminateThreadTimeout exception is
thrown.

> Is the code below acceptable

No.

> In other words is threading prepared for me automatically
> (contrary to TServerSocket where I should spawn the new
> thread from OnGetThread() )?

TIdTCPServer handles the threading for you, yes.

> 5) TServerSocket can stay in 'Active == true' but unable
> to receive new requests in some circumstances.

You are referring to when the listening thread crashes, correct?

> Is that the same at TIdTCPServer?

If an unexpected exception occurs while accepting a new connection, the
server aborts the connection and closes it, then triggers its
OnListenException event, and then continues listening for new connections as
if nothing happened.  The only way for the server to stop listening
unexpectedly is if an exception is thrown while the aborted connection is
being cleaned up.

> And if that's true, how should I protect TIdTCPServer::OnExecute()
> from stopping functioning?

You don't, because that is not the correct place to do that.  OnExecute is
already protected from uncaught exceptions.  If an exception escapes the
OnExecute event handler, the connection with the client is closed and the
server contnues listening for new connections.

> How to know the number of active connections/threads?

Use the Threads property, ie:

    TList *List = IdTCPServer1->Threads->LockList;
    // look at List->Count
    IdTCPServer1->Threads->UnlockList;

> To be more precise, I want to limit the heavy operations
> in TIdTCPServer::OnExecute() so I want to do something
> like this /PSEUDO/:

You are taking the wrong approach.  To limit the number of allowed
connections, simply set the TIdTCPServer's MaxConnections property before
activating the server.  Unlike TServerSocket, TIdTCPServer keeps track of
the number of connections and automatically closes them when too many are
connected.  TIdTCPServer also has a MaxConnectionReply property that can be
used to send a reply back to the client telling it that there are too many
connections.

Also, your code suggests that you are not aware that the OnExecute event is
not called once per connection.  It is called over and over.  Unlike
TServerSocket, where you have to implement your own looping when you write a
thread class, TIdTCPServer loops the OnExecute event automatically.  So, if
you had TServerSocket code like the following:

    void __fastcall TMyThread::ClientExecute()
    {
        //...
        while( !Terminated && ClientSocket->Connected )
        {
            // read a block...
            // write a block...
        }
        //...
    }

Your TIdTCPServer code would look like this:

    void __fastcall TForm1::IdTCPServer1Execute(AThread: TIdPeerThread)
    {
        // read a block...
        // write a block...
    }

>       // Sending the request length to server ...
>       IdTCPClient->WriteInteger( REQUEST.Length() );

Why are you sending the string's length if you are not going to send the
string directly?

>       TStringList *SL = new TStringList;
>       SL->Add( REQUEST );
>       IdTCPClient->WriteStrings( SL );
>       delete SL;

Why are you using a TStringList to send a single string?  That is very
wasteful of memory.  Not only that, but WriteStrings() will write more data
than you have specified with WriteInteger().  Specifically, WriteStrings()
uses WriteLn(), so your request will have an extra CRLF at the end of it.
Based on the server code you have shown, you need to use Write() instead of
WriteStrings():

    // Sending the request to server ...
    IdTCPClient->WriteInteger( REQUEST.Length() );
    IdTCPClient->Write( REQUEST );

>    // Receive request ...
>    AnsiString REQUEST = AThread->Connection->ReadString( BytesToRead );

That will not work because WriteStrings() sent an extra CRLF for each line
that you are not reading.  That CRLF will remain in the socket until the
next read, which will then corrupt the reading of the integer value for the
next request.

>    Memo->Lines->Add( REQUEST + " (received)" );
<snip>
>    Memo->Lines->Add( RESPONSE + " (prepared)" );
<snip>
>    Memo->Lines->Add( RESPONSE + " (sent)" );

That is not thread-safe on the server side.

>    // Send to respondse to client ...
>    TStringList *SL = new TStringList;
>    SL->Add( RESPONSE );
>    AThread->Connection->WriteStrings( SL );
>    delete SL;

Again, that is very wasteful of memory, and will corrupt the communication
because of the extra CRLF that you are not taking into account.

Now, with all of that said, I suggest a different approach - make use of
Indy's CommandHandler system.  For example:

    --- Client ---

    void __fastcall TIndyClientForm::AskButtonClick(TObject *Sender)
    {
        AskButton->Enabled = false;

        try
        {
            IdTCPClient->Connect(5000);
            try
            {
                IdTCPClient->SendCmd("TIME", 200);
                Memo->Lines->Add(IdTCPClient->LastCmdResult->Text);
            }
            __finally
            {
                IdTCPClient->Disconnect();
            }
        }
        catch(const Exception &e)
        {
            Memo->Lines->Add("ERROR: " + e.Message);
        }

        AskButton->Enabled = true;
    }


    --- Server ---

    __fastcall TIndyServerForm::TIndyServerForm(TComponent *Owner)
        : TForm(Owner)
    {
        // you can set this stuff up at design-time instead...
        TIdCommandHandler *Command = IdTCPServer->CommandHandlers->Add();
        Command->Command = "TIME";
        Command->OnCommand = CommandTIME;
        Command->ReplyNormal->NumericCode = 200;
        Command->ReplyExceptionCode = 500;
        Command->ParseParams = false;

        IdTCPServer->Active = true;
    }

    void __fastcall TIndyServerForm::CommandTIME(TIdCommand *ASender)
    {
        ASender->Reply->SetReply(200, Now().FormatString("hh:mm AM/PM"));
    }


Gambit

"Vladimir Stefanovic" <antivari@po.sbb.co.yu> wrote in message
news:43452c1b@newsgroups.borland.com...

> After every 'ASK' button click, the reasponse is
> slower and slower. Why?

Because your code has a major bug in it.  See my other reply.


Gambit

"Vladimir Stefanovic" <antivari@po.sbb.co.yu> wrote in message
news:434534b1@newsgroups.borland.com...

> I think I have found the 'bug'.

No, you have not.  See my other reply.

> I thought that:
>    IdTCPClient->Disconnect();
> ... will also close the thread (connection) on
> server side.

It will.


Gambit

> Since you appear to be using Indy 9, then Connect()
> has an ATimeout parameter.

Hm... I'm using Indy that was shipped with BCB6 and
I just cannot find 'ATimeout' parameter in Connect()?

At line:
   IdTCPClient->Connect(5000);
... it says: "Extra parametar ..."

What am I doing wrong? I thought it's Indy 9!

>> How to know the number of active connections/threads?
>
> Use the Threads property, ie:
>
>    TList *List = IdTCPServer1->Threads->LockList;
>    // look at List->Count
>    IdTCPServer1->Threads->UnlockList;

Why you are using LockList/UnlockList? Is that a kind
of protection when getting 'Count'?

>> To be more precise, I want to limit the heavy operations
>> in TIdTCPServer::OnExecute() so I want to do something
>> like this /PSEUDO/:
>
> TIdTCPServer also has a MaxConnectionReply property
> that can be used to send a reply back to the client telling
> it that there are too many connections.

This is interesting, but right now I cannot ask you anything
before I try it in practice...

> Also, your code suggests that you are not aware that the
> OnExecute event is not called once per connection.  It is
> called over and over.

I thought that ONE 'Connect()' on client side triggers ONE
'OnExecute()' on server side... But now, when I re-think,
it's obvious that *cannot* be possible - that I say on client side:

   WriteInteger()
   WriteString()

...and expect that *only* ONE OnExecute() is trigered.

But is it true that ONE Write...() trigeres ONE OnExecute()?


>>       // Sending the request length to server ...
>>       IdTCPClient->WriteInteger( REQUEST.Length() );
>
> Why are you sending the string's length if you are not going to
> send the string directly?

Because I overlooked Write() method.

>>       TStringList *SL = new TStringList;
>>       SL->Add( REQUEST );
>>       IdTCPClient->WriteStrings( SL );
>>       delete SL;
>
> Why are you using a TStringList to send a single string?
> [...].
> Based on the server code you have shown, you need to use
> Write() instead of WriteStrings():

Because I overlooked Write() method. I searched for:
WriteString(), not WriteStrings() but there wasn't it.

I just overlooked Write(), that's why I needed to send
the length of a string, and the argument of WriteStrings()
was TString*, and that's why I needed a StringList.


>>    Memo->Lines->Add( REQUEST + " (received)" );
> <snip>
>>    Memo->Lines->Add( RESPONSE + " (prepared)" );
> <snip>
>>    Memo->Lines->Add( RESPONSE + " (sent)" );
>
> That is not thread-safe on the server side.

Yes, I was aware of that... I just wanted to resolve major
problems first...

> Now, with all of that said, I suggest a different approach
> - make use of Indy's CommandHandler system.
> < CODE >

Thank you for the code. I must read about that, but it looks
very useful.




-- 
Best regards,
Vladimir Stefanovic 

Remy,

I modified my code to look like this, and now, really,
only the client closes the connection.

Also I used Write() instead of WriteStrings() and no
slow downs are present any more...

For the purpose of this example I didn't protected:

   Memo->Lines->Add()

... but I will in the real life.

I'm interested is the process of Writing/Reading
correct now?


--- Server ---

void __fastcall TIndyServerForm::IdTCPServerExecute(TIdPeerThread *AThread)
{
   // RECEIVING TASKS ...
   int BytesToRead = AThread->Connection->ReadInteger();
   AnsiString REQUEST = AThread->Connection->ReadString( BytesToRead );
   Memo->Lines->Add( REQUEST + " (received)" );

   // PROCESSING & PREPARING ANSWER TASKS ...
   AnsiString RESPONSE = FormatDateTime( "hh:mm AM/PM", Now() );
   Memo->Lines->Add( RESPONSE + " (prepared)" );

   // SENDING TASKS ...
   AThread->Connection->WriteInteger( RESPONSE.Length() );
   AThread->Connection->Write( RESPONSE );
   Memo->Lines->Add( RESPONSE + " (sent)" );
}


--- Client ---

void __fastcall TIndyClientForm::AskButtonClick(TObject *Sender)
{
   AskButton->Enabled = false;

   try
   {
      // CONNECTING
      IdTCPClient->Connect();

      // PREPARING REQUEST
      AnsiString REQUEST = "Can you please tell me what's the time?";
      Memo->Lines->Add( REQUEST + " (prepared)" );

      // SENDING TASKS
      IdTCPClient->WriteInteger( REQUEST.Length() );
      IdTCPClient->Write( REQUEST );
      Memo->Lines->Add( REQUEST + " (sent)" );

      // RECEIVING TASKS
      int BytesToRead = IdTCPClient->ReadInteger();
      AnsiString RESPONSE = IdTCPClient->ReadString( BytesToRead );
      Memo->Lines->Add( RESPONSE );
   }
   __finally
   {
      IdTCPClient->Disconnect();
      AskButton->Enabled = true;
   }
}


-- 
Best regards,
Vladimir Stefanovic 

"Vladimir Stefanovic" <antivari@po.sbb.co.yu> wrote in message
news:43456cbf@newsgroups.borland.com...

> I'm using Indy that was shipped with BCB6

Then you are using Indy 8, which is an outdated version that is no longer
supported.  Please go to Indy's website (http://www.indyproject.org) and
upgrade to Indy 9 or 10.

> What am I doing wrong? I thought it's Indy 9!

No, it is not.  It is Indy 8.

> Why you are using LockList/UnlockList?

Because the Threads property is a TThreadList, which is a TList that is
protected by a critical section.  You have to lock the critical section in
order to access the TList for any reason.

> I thought that ONE 'Connect()' on client side triggers
> ONE 'OnExecute()' on server side...

Nope.  OnExecute is triggered in a loop inside of TIdTCPServer, ie:

    TIdPeerThread::Run()
    {
        //...
        while( !Stopped )
            OnExecute(this);
        //...
    }

You can use the TIdPeerThread's Connection property to know which client is
triggering the event handler at the time.

> But now, when I re-think, it's obvious that *cannot* be
> possible - that I say on client side:
>
>    WriteInteger()
>    WriteString()
>
> ...and expect that *only* ONE OnExecute() is trigered.

That is not what I meant.  Individual writing operations do not trigger
their own OnExecute events.  When the OnExecute event is triggered, you read
everything you need from it, and then exit.  The OnExecute is then looped,
waiting for you to read more data when ready.

> But is it true that ONE Write...() trigeres ONE OnExecute()?

No.

> Yes, I was aware of that... I just wanted to resolve major
> problems first...

Thread safety is a major problem.  You can deadlock or crash the server if
your code is not thread-safe.


Gambit

"Vladimir Stefanovic" <antivari@po.sbb.co.yu> wrote in message
news:434574d8$1@newsgroups.borland.com...

> I'm interested is the process of Writing/Reading
> correct now?

For now, although as I suggested earlier, you should wrap the
Connect/Disconnect() in its own try..finally block.  Don't call Disconnect()
if Connect() fails.


Gambit

>  Don't call Disconnect() if Connect() fails.

Thank you.

-- 
Best regards,
Vladimir Stefanovic 

>  Please go to Indy's website (http://www.indyproject.org)
> and upgrade to Indy 9 or 10.

What do you suggest me? Is there any penalties when
using 9 or 10 with BCB6 ?

> Thread safety is a major problem.  You can deadlock or
> crash the server if your code is not thread-safe.

OK, I'll think of that.




-- 
Best regards,
Vladimir Stefanovic