Re: thread safe functions

[Andy Wingo <wingo@pobox.com>]

Hey Ken,

I got tired of seeing your mail marked as starred in my inbox, so I
decided to take a look at it ;-)

You were writing about threadsafe access to global variables.

On Sun 29 Aug 2010 03:33, Ken Raeburn <raeburn@raeburn.org> writes:

> looking at deprecation.c, I'd suggest not printing stuff while the
> lock is held

Fixed, thanks.

> I was starting to look through the port code for this, but got
> distracted by scm_ptobs, a pointer which appears to be dynamically
> reallocated as needed when scm_make_port_type is called.  Which means
> every function that reads it should be locking the same mutex used when
> updating it; in this case, that appears to be the generic "critical
> section" stuff, and it looks like we're not doing that.

Yeah, and it is exported to users of libguile.  Fun, no?  I added a
comment to the header about its impending deprecation, but that's all I
have time for right now.

> foreign.c: pointer_weak_refs is a weakly keyed hash table, which we add
> things to without locking.

Fixed.

> instructions.c: Can scm_lookup_instruction_by_name be called the "first"
> time from two threads concurrently?

Yes; fixed.

> modules.c: scm_pre_modules_obarray doesn't look protected.

Doesn't need to be; added a comment to that effect.

> ports.c: scm_i_port_weak_hash is protected in some places, others I
> haven't investigated further.  But in scm_c_port_for_each, it looks to
> me as though, if ports are being created while the function is setting
> up, you could end up raising an exception if scm_internal_hash_fold
> iterates over more ports than were in the hash table when the count was
> initially taken (and the vector size set).

Fixed scm_c_port_for_each, and access to scm_i_port_weak_hash.  But the
scm_ptobs situation is as unresolved as ever.

> procproc.c: There's a mutex to protect overrides, but it looks like
> set-procedure-property! doesn't use it correctly; it should look more
> like set-object-property! does.

I'm going to punt on this one, since it cannot access the hash table in
an inconsistent state, and I don't care enough about what happens when
multiple threads start changing the same procedure property (a fairly
legacy interface) at once.  Furthermore there is always (assq-set!
(procedure-properties foo) 'bar) to contend with...

> properties.c: I'm not familiar with this code at all, but I'd guess
> properties_whash should be protected.  Though I wonder if the call-out
> from primitive-property-ref could invoke other primitive-property
> functions, causing a deadlock if it isn't done carefully.

I went ahead and deprecated this whole mess, it was only used for object
properties, which I reimplemented using weak hash tables and a lock.

> symbols.c: I don't think 'symbols' is handled safely.  But this code is
> all starting to run together in my mind. :-)

I think I fixed this one a month ago or so.

> smob.c: I don't think tramp_weak_map is adequately protected, but I'm
> not certain.
>
> srcprop.c: scm_source_whash isn't protected; it also appears to be
> exposed by the name "source-whash" to Scheme code, which wouldn't be
> able to use a mutex defined and used purely in the C code.
>
> (What's that, four different kinds of property-list mappings, all
> implemented separately?)

Tell me about it...

> struct.c: Even calling struct-vtable-name can cause a hash table entry
> to be created, it appears.  So that's not thread-safe, never mind the
> call to actually change the name.
>
> weaks.c: Is only making things for Scheme code, where we're making
> locking the user's problem.

At this point I got tired.  But sending this mail to the list will
requeue this work, so hey...

> It would be a bit easier if hash tables themselves were thread-safe.  A
> read/write lock might make more sense than a simple mutex though, if
> reading is more common than writing.  In my experience they sometimes
> perform worse than simple mutexes, so they're a win mainly for
> concurrent reading.

Yeah, that might be a good idea. For later, though.

> Of course, hash tables aren't the only places where we can have
> problems, and I'm not even touching on hash tables that might be created
> by Scheme code (but exposed to users with APIs that don't scream "unsafe
> hash table used here!"), but I've already spent my afternoon and evening
> on this.

Indeed, and I just spent some hours on it too...

>>> And don't get me going on memory ordering models....
>> 
>> I'm interested!

Thanks for the notes on this, it was an interesting read.

> A real review should check:
>  * any non-const static-storage variables defined in the library, and
> how they're used;
>  * any functions modifying objects, to determine the effect of
> concurrent calls;
>  * any uses of those functions or objects from our C or Scheme code, to
> see if we're presenting the correct semantics in the face of concurrent
> calls;
>  * that we actually specify the semantics, or when results are
> undefined, clearly;
>  * anything dealing with OS interfaces (or other library APIs) that
> aren't thread-safe;
> And if those aren't going to take long enough already:
>  * getting *real* answers to the memory ordering issues;
>  * possibly reviewing every function manipulating memory for ordering
> issues;
>  * probably more.
>
> Then, I might have some confidence that "things are mostly there."  I
> don't think it's going to happen at all for 2.0, and I don't expect to
> have time myself to do anything more than little bits of work on the
> first part of the list, if even that much, in the near future.

Indeed, it certainly won't all happen for 2.0.  Maybe during 2.0 and
changes for 2.2.  Keep the reviews coming, though...

Cheers,

Andy
-- 
http://wingolog.org/