GStack.LocalAddresses not (thread) safe

[Yahoo Serious]

We use the global IdStack.GStack.LocalAddresses to get information about
local IP-addresses.

We sometimes use it from multiple threads. However, it seems that
TIdStack.LocalAddresses is not thread safe:
It is possible that thread A is using the returned LocalAddresses, e.g.:
A: MyAdresses.Assign(GStack.LocalAddresses)
but while copying the list thread B may be changing it:
B: MyAdresses.Assign(GStack.LocalAddresses)
because TIdStack.GetLocalAddresses does a:
FLocalAddresses.Clear;
So thread A may be referencing an item-index that B has just erased.

This is fortified by the LocalAddresses varying more these days, due to a.o.
VPN and BlueTooth. So even if the list is filled again by thread B, the
previous Count (returned in thread A) may differ from the present Count
(resulting from thread B).

This last problem is also possible in a single thread, if people access
LocalAddresses multiple times in a single procedure. People probably don't
realize this causes the list to be repopulated. E.g. looking at DejaNews
for earlier reports, I saw Don Siders (Team Indy!) suggest this code (in
2001):
---
// create listeners for each IP address on port 2112
for (iAddr = 0; iAddr < GStack->LocalAddresses->Count; iAddr++)
{
AServer->Bindings->Add();
AServer->Bindings->Items[iAddr]->IP = GStack->LocalAddresses[iAddr];
AServer->Bindings->Items[iAddr]->Port = 2112;
}
---
If the Count of LocalAddresses in the second call, has been decreased due to
the repopulationg, this will
cause an AV.

So I think this should be resolved, by either:
- make FLocalAddresses a thread safe list
- make GStack a threadvar
- make [Get]LocalAddresses a function with parameter (const ARepopulate:
boolean = False)

What do you think?

Yahoo

[Remy Lebeau (TeamB)]

Post by Yahoo Serious
We sometimes use it from multiple threads. However, it seems

No, it is not. In your code, you will have to wrap access to it in a
critical section manually.

Post by Yahoo Serious
This last problem is also possible in a single thread, if people access
LocalAddresses multiple times in a single procedure. People probably
don't realize this causes the list to be repopulated.

Then the documentation needs to be updated to say as much.

Post by Yahoo Serious
for earlier reports, I saw Don Siders (Team Indy!) suggest this

Change that to the following instead:

// create listeners for each IP address on port 2112
TStrings *LocalAddrs = GStack->LocalAddresses;
for (iAddr = 0; iAddr < LocalAddrs->Count; ++iAddr)
{
TIdSocketHandle *Binding = AServer->Bindings->Add();
Binding->IP = LocalAddrs->Strings[iAddr];
Binding->Port = 2112;
}

Post by Yahoo Serious
- make FLocalAddresses a thread safe list

Could. But for apps that use Indy in a single thread, that introduces
unneeded overhead.

Post by Yahoo Serious
- make GStack a threadvar

Can't do that. GStack has to point to a single global object that is shared
by all threads.

Post by Yahoo Serious
- make [Get]LocalAddresses a function with parameter
(const ARepopulate: boolean = False)

That would require C++ users to re-write their code.


Gambit

[Yahoo Serious]

Post by Remy Lebeau (TeamB)
Then the documentation needs to be updated to say as much.

Please also document that TIdStack.LocalAddresses is not thread safe (and
programmers will have to fix this manually, with a critical section or
whatever)

Post by Remy Lebeau (TeamB)
// create listeners for each IP address on port 2112
TStrings *LocalAddrs = GStack->LocalAddresses;
for (iAddr = 0; iAddr < LocalAddrs->Count; ++iAddr)
{
TIdSocketHandle *Binding = AServer->Bindings->Add();
Binding->IP = LocalAddrs->Strings[iAddr];
Binding->Port = 2112;
}

This is a solution for a single thread, but for multiple threads you still
need to use a critical section manually. (I'm a Delphi programmer, but I
think I am reading this correctly: you create a pointer, so this thread does
not trigger Repopulate, but some other thread might.)

Post by Remy Lebeau (TeamB)

Post by Yahoo Serious
- make FLocalAddresses a thread safe list

Could. But for apps that use Indy in a single thread, that introduces
unneeded overhead.

I agree, especially since I think the need for repopulation will usually be
rare. (But I'm programming pretty static C/S applications, no dynamic
envrionments like games/chats/gadgets)

Post by Remy Lebeau (TeamB)

Post by Yahoo Serious
- make GStack a threadvar

Can't do that. GStack has to point to a single global object that is
shared by all threads.

Okay.

Post by Remy Lebeau (TeamB)

Post by Yahoo Serious
- make [Get]LocalAddresses a function with parameter
(const ARepopulate: boolean = False)

That would require C++ users to re-write their code.

And other coders too.
Yes, I realize that, but I think the possibility for ('cheap') safety is
worth the backward incompatibility. Another option might be to add it as an
overloaded 'thread safe' possibility.

Yahoo.

[Remy Lebeau (TeamB)]

Post by Yahoo Serious
Please also document that TIdStack.LocalAddresses is not thread safe

It is the same thing. GStack is just a global TIdStack pointer. So only
the TIdStack documentation needs updating.

Post by Yahoo Serious
This is a solution for a single thread, but for multiple threads you still
need to use a critical section manually.

Yes. But the code change I suggested lends itself better to that, ie:

CRITICAL_SECTION GStackLocalAddressesLock;

... thread A ...

// create listeners for each IP address on port 2112
EnterCriticalSection(&GStackLocalAddressesLock);
try
{
TStrings *LocalAddrs = GStack->LocalAddresses;
for (iAddr = 0; iAddr < LocalAddrs->Count; ++iAddr)
{
TIdSocketHandle *Binding = AServer->Bindings->Add();
Binding->IP = LocalAddrs->Strings[iAddr];
Binding->Port = 2112;
}
}
__finally
{
LeaveCriticalSection(&GStackLocalAddressesLock);
}

... thread B ...

// create listeners for each IP address on port 2112
EnterCriticalSection(&GStackLocalAddressesLock);
try
{
TStrings *LocalAddrs = GStack->LocalAddresses;
// use LocalAddrs as needed...
}
__finally
{
LeaveCriticalSection(&GStackLocalAddressesLock);
}

Post by Yahoo Serious
you create a pointer, so this thread does not trigger Repopulate

I assign a pointer to point at the TStringList object that LocalAddresses
returns so that the loop does not repopulate the list again, yes. In
Delphi, it would be the following:

var
LocalAddrs: TStrings;
iAddr: Integer;
begin
// create listeners for each IP address on port 2112
LocalAddrs := GStack.LocalAddresses;
for iAddr := 0 to Pred(LocalAddrs.Count) do
begin
with AServer.Bindings.Add do
begin
IP := LocalAddrs[iAddr];
Port := 2112;
end;
end;
end;

Post by Yahoo Serious
but some other thread might.

If you don't lock access to it, yes.

Post by Yahoo Serious
Another option might be to add it as an overloaded 'thread safe'
possibility.

A better option would be to introduce a new method that merely fills in a
TStrings that is passed in as input, rather than repopulating an internal
TStringList and returning a pointer to it. That way, each thread can
allocate its own TStringList and use it regardless of other threads.


Gambit

[Remy Lebeau (TeamB)]

Post by Remy Lebeau (TeamB)
A better option would be to introduce a new method that merely fills in a
TStrings that is passed in as input, rather than repopulating an internal
TStringList and returning a pointer to it. That way, each thread can
allocate its own TStringList and use it regardless of other threads.

I have added a new AddLocalAddressesToList() method to TIdStack for this.


Gambit