Re: Threading problem in IconOverlay code

Tomas Kopal <Tomas.Kopal@altap.cz> wrote:

> And here comes the problem. In file IconOverlay.cpp in TortoiseShell,
> there is a COM method CShellExt::IsMemberOf. This method is using some
> global variables, e.g. g_filepath, and does not use any synchronization.

Thank's a lot for figuring that out!

Well, the shell integration is one of the oldest parts of TSVN
and has lived through much code churn. As a result, some quick-
and-dirty hacks from its infanty prevailed while other structures /
decissions have become obsolete but not cleaned-up.

> Possible solutions? I see threee:
> - Add synchronization to this and other methods using global variables.
> The simplest, but least effective and clean.

Actually, this is not too inefficient (critical sections have only
a few nanoseconds of overhead). The main problem is that the way
these variables are used: temporary storage for local(?) state
across method calls.

> - Change the global variables to be member variables of the COM object,
> thus making them per-instance.

This one is clearly to be prefered. g_filepath, for instance, is
only used in IsMemberOf (I simply told VS to grep all files). Thus,
it can safely be made a member variable. My impression is that this
is true for most globals.

> - Change the global variables to be stored in TLS (thread local
> storage). Clean, may be better for the effectivity of the caching
variables.

May or may not be suitable. There might be places where an actual
singleton is required. However, TSVN is unlikely to handle that
semantically correctly (i.e. does not expect someone else to change
the state of the singleton).

So, patches are welcome ;) I think we can improve the shell
integration by simply making most globals members.

-- Stefan^2.
Received on Thu Jun 28 11:45:20 2007