How to synchronize TIdHTTPServer::OnCommandGet with VCL-CodePudding

I have this code in a VCL Forms application, written in C Builder XE6, which is not thread-safe:

void __fastcall TForm1::HTTPServerCommandGet (TIdContext *AContext, TIdHTTPRequestInfo *ARequestInfo, TIdHTTPResponseInfo *AResponseInfo)
{
    ListHistory->Items->BeginUpdate();

    try
    {
        TListItem* Item = ListHistory->Items->Add();
        Item->Caption = Now().FormatString(_D("dd/mm/yyyy hh:nn:ss"));
        Item->SubItems->Add(_D(""));
        Item->SubItemImages[0] = 0; // 0 = Incoming
        Item->SubItems->Add(ARequestInfo->RemoteIP);
        Item->SubItems->Add(ARequestInfo->Command   _D(" ")   ARequestInfo->URI);
        Item->SubItems->Add(ARequestInfo->Version);
        Item->SubItems->Add(ARequestInfo->RawHeaders->CommaText);
        if (ARequestInfo->PostStream)
            Item->SubItems->Add(ReadStringFromStream(ARequestInfo->PostStream));
        else
            Item->SubItems->Add(_D(""));
    }
    __finally
    {
        ListHistory->Items->EndUpdate();
    }

    // Process request.
    ...

    ListHistory->Items->BeginUpdate();

    try
    {
        TListItem* Item = ListHistory->Items->Add();
        Item->Caption = Now().FormatString(_D("dd/mm/yyyy hh:nn:ss"));
        Item->SubItems->Add(_D(""));
        Item->SubItemImages[0] = 1; // 1 = Outgoing
        Item->SubItems->Add(AContext->Binding->IP);
        Item->SubItems->Add(IntToStr(AResponseInfo->ResponseNo)   _D(" ")   AResponseInfo->ResponseText);
        Item->SubItems->Add(ARequestInfo->Version);
        Item->SubItems->Add(AResponseInfo->RawHeaders->CommaText);
        Item->SubItems->Add(AResponseInfo->ContentText);
    }
    __finally
    {
        ListHistory->Items->EndUpdate();
    }
}

ListHistory is a TListView.

To make this code thread-safe, I need to synchronize access to all VCL components with the main thread.

I googled and searched StackOverflow, and found a lot of examples for Delphi that use anonymous methods with TThread.Synchronize(), but none for C Builder. I also found that TIdSync and TIdNotify are now deprecated and wrappers for TThread::Synchronize() and TThread::Queue(). That would make Synchronize() the method to use in my case.

What is the simplest way to make the above code thread-safe?

Can I simply create a TListItem in the OnCommandGet handler, store it as a data member of the TForm1 class and call Synchronize() on a parameterless method of the TForm1 class, that adds the stored TListItem to ListHistory? Or would that still not be thread-safe?

Do I need to store all item/subitem strings and the image index separately as data members of TForm1?

Or, is there a better way to do this?

CodePudding user response：

I googled and searched StackOverflow, and found a lot of examples for Delphi that use anonymous methods with TThread.Synchronize(), but none for C Builder.

This is covered in Embarcadero's documentation:

How to Handle Delphi Anonymous Methods in C

In a nutshell, if you are using one of the Clang-based compilers, you can simply pass a C lambda to TThread::Synchronize(). On the other hand, if you are using the "classic" Borland compiler instead, you can either:

define a class that implements the TThreadProcedure interface and its Invoke() method, and then pass an instance of that class to the _di_TThreadProcedure overload of TThread::Synchronize().
define a class that implements a parameter-less method, and then create an instance of that class and pass a pointer to the method to the TThreadMethod overload of TThread::Synchronize().

I also found that TIdSync and TIdNotify are now deprecated and wrappers for TThread::Synchronize() and TThread::Queue().

Correct. However, even though they are deprecated, they have not been removed yet (that will come in Indy 11 or 12), so you can use them if you want to, for now.

Can I simply create a TListItem in the OnCommandGet handler, store it as a data member of the TForm1 class and call Synchronize() on a parameterless method of the TForm1 class, that adds the stored TListItem to ListHistory? Or would that still not be thread-safe?

No, that would not be thread-safe. OnCommandGet is called in a worker thread, and you can't just "create a TListItem" without going through the TListView and its underlying HWND, so that would need to be created in the main UI thread. And, if you have multiple clients connected, you would need to serialize access to such a data member in the Form, causing a race condition.

Do I need to store all item/subitem strings and the image index separately as data members of TForm1?

That is one way to go. However, if you have multiple clients connected, you would need to serialize access to those data members.

So, it is better to keep the values isolated for each event.

Or, is there a better way to do this?

If you are using the "classic" compiler, I would suggest doing what TIdSync does internally. Define a class with the data members you need, and then define a parameter-less method in that class to use the data members, and pass that method to TThread::Synchronize(), eg:

class TAddToListHistory
{
public:
    TListView *History;
    TIdContext *Context;
    TIdHTTPRequestInfo *Request;
    TIdHTTPResponseInfo *Response;

    void __fastcall AddRequest()
    {
        History->Items->BeginUpdate();
        try
        {
            TListItem* Item = History->Items->Add();
            Item->Caption = Now().FormatString(_D("dd/mm/yyyy hh:nn:ss"));
            Item->SubItems->Add(_D(""));
            Item->SubItemImages[0] = 0; // 0 = Incoming
            Item->SubItems->Add(Request->RemoteIP);
            Item->SubItems->Add(Request->Command   _D(" ")   Request->URI);
            Item->SubItems->Add(Request->Version);
            Item->SubItems->Add(Request->RawHeaders->CommaText);
            if (Request->PostStream)
                Item->SubItems->Add(ReadStringFromStream(Request->PostStream));
            else
                Item->SubItems->Add(_D(""));
        }
        __finally
        {
            History->Items->EndUpdate();
        }
    }

    void __fastcall AddResponse()
    {
        History->Items->BeginUpdate();
        try
        {
            TListItem* Item = History->Items->Add();
            Item->Caption = Now().FormatString(_D("dd/mm/yyyy hh:nn:ss"));
            Item->SubItems->Add(_D(""));
            Item->SubItemImages[0] = 1; // 1 = Outgoing
            Item->SubItems->Add(Context->Binding->IP);
            Item->SubItems->Add(IntToStr(Response->ResponseNo)   _D(" ")   Response->ResponseText);
            Item->SubItems->Add(_D("HTTP/1.1")/*Request->Version*/);
            Item->SubItems->Add(Response->RawHeaders->CommaText);
            Item->SubItems->Add(Response->ContentText);
        }
        __finally
        {
            History->Items->EndUpdate();
        }
    }
};

void __fastcall TForm1::HTTPServerCommandGet(TIdContext *AContext,
    TIdHTTPRequestInfo *ARequestInfo, TIdHTTPResponseInfo *AResponseInfo)
{
    TAddToListHistory adder;
    addr.History = ListHistory;
    addr.Context = AContext;
    addr.Request = ARequestInfo;
    addr.Response = AResponseInfo;

    TThread::Synchronize(NULL, &addr.AddRequest);

    // Process request.
    ...

    TThread::Synchronize(NULL, &addr.AddResponse);
}

If you are using a Clang-based compiler, then using lambdas would be simpler, eg:

void __fastcall TForm1::HTTPServerCommandGet(TIdContext *AContext,
    TIdHTTPRequestInfo *ARequestInfo, TIdHTTPResponseInfo *AResponseInfo)
{
    TThread::Synchronize(nullptr,
        [=]{
            ListHistory->Items->BeginUpdate();
            try
            {
                TListItem* Item = ListHistory->Items->Add();
                Item->Caption = Now().FormatString(_D("dd/mm/yyyy hh:nn:ss"));
                Item->SubItems->Add(_D(""));
                Item->SubItemImages[0] = 0; // 0 = Incoming
                Item->SubItems->Add(ARequestInfo->RemoteIP);
                Item->SubItems->Add(ARequestInfo->Command   _D(" ")   ARequestInfo->URI);
                Item->SubItems->Add(ARequestInfo->Version);
                Item->SubItems->Add(ARequestInfo->RawHeaders->CommaText);
                if (ARequestInfo->PostStream)
                    Item->SubItems->Add(ReadStringFromStream(ARequestInfo->PostStream));
                else
                    Item->SubItems->Add(_D(""));
            }
            __finally
            {
                ListHistory->Items->EndUpdate();
            }
        }
    );

    // Process request.
    ...

    TThread::Synchronize(nullptr,
        [=]{
            ListHistory->Items->BeginUpdate();
            try
            {
                TListItem* Item = ListHistory->Items->Add();
                Item->Caption = Now().FormatString(_D("dd/mm/yyyy hh:nn:ss"));
                Item->SubItems->Add(_D(""));
                Item->SubItemImages[0] = 1; // 1 = Outgoing
                Item->SubItems->Add(AContext->Binding->IP);
                Item->SubItems->Add(IntToStr(AResponseInfo->ResponseNo)   _D(" ")   AResponseInfo->ResponseText);
                Item->SubItems->Add(_D("HTTP/1.1")/*ARequestInfo->Version*/);
                Item->SubItems->Add(AResponseInfo->RawHeaders->CommaText);
                Item->SubItems->Add(AResponseInfo->ContentText);
            }
            __finally
            {
                ListHistory->Items->EndUpdate();
            }
        }
    );
}