[GBBopen-developer] Portable threads fix for SCL

[Dan Corkill]

Douglas Crosher wrote:

> Some suggestions for the portable threads library:

Thanks for taking the time to share these suggestions.  Here are my
comments (and sometimes explanations) about them.  Comments and suggestions
from others are welcomed!

> all-threads: this function is of limited use because threads may
> exit after the call but before the result is used.  The SCL has a
> function to map over all threads and ensure they do not exit.
> 
> thread-alive-p: this function is of limited use because the thread
> may exit after the call but before the result is used in which case
> it would be wrong.  Of some use for development and diagnosis.

As you note, ALL-THREADS and THREAD-ALIVE-P are mainly for debugging
purposes (there are others, too!).  One of the objectives of the Portable
Threads interface is to provide a uniform interface to such utility
functions.  Adding caution notes about such thread-state changes to the
documentation of these functions would be a good idea (someday soon?).

> with-timeout: this is generally unsafe and should not be used in
> deployed applications.  It is also of limited development value
> so I suggest just removing it.

Timeouts (such as WITH-TIMEOUT) provide a strong rope that people can
certainly hang themselves with.  Support for WITH-TIMEOUT was a late
addition to Portable Threads because there was demand for such rope.  When
used with care (in terms of the timed form with possibly unwind forms),
there are some situations where it is useful.

> %%get-lock%%, condition-variable: is there a real need for locks
> and condition variables to be standard-objects?  Wouldn't
> applications just be able to encapsulate them in standard-objects
> if necessary?  Perhaps the intention is that classes include the
> lock class as a mixin class for objects needing a lock, but then
> what about an object needing two or more locks?  The design also
> requires 'with-lock-held etc to work on condition variable, but
> condition-variables are not locks...  Suggest keeping it simple
> and separating locks and condition variables, then there is no
> need for the standard-objects and the %%get-lock%% hack.

Portable Threads condition variables are a higher-level concept than
OS-level primitives.  This is a "lispy" Portable Threads design decision
that not everyone will agree with.  Predictable scheduling requires an
association between a mutex and the OS-level condition variable.  The
Portable Threads design enforces this by unifying the lock as part of the
condition variable object--eliminating the need for the programmer to
maintain this association.  Early in the design we considered naming this
higher-level concept object as something other than a condition variable
(and also providing a lower-level bare bones interface to OS-level
condition variables), but not all CL implementations supported OS-level
condition variables.  Eventually, we settled on the "lispy" packaging and
the Portable Threads users at the time thought maintaining the "condition
variable" name was the best (even though the name is too close to CL's
conditions to suit some).

As you note, if an object managed with a Portable Threads condition
variable needs additional locks, they can be added to the object as either
addition locks (or additional condition variables).  Proper ordering
(contouring) of lock/cv acquisition by the programmer would still be
required to avoid deadlock and maintain predictable scheduling in such cases.

> make-lock: The SCL also supports read-write locks.

At present, the Portable Threads interface does not extend to read-write
lock behavior, as this would need to be supported/simulated on all
platforms (volunteers?).  We certainly could extend the Portable Threads
MAKE-LOCK interface function to pass-through CL-implementation-specific
lock parameters.

> thread-holds-lock-p: The SCL is built on POSIX threads which does
> does not support this operation.

This is another function that some find very useful during development &
debugging--even if it is a holdover from the MP days.  Even with OS-level
threads, we are trying to provide this aid when we can.

> atomic-*: The SCL tries to use an atomic instructions to update
> the place which is fast and safe but it only supports a limited
> range of places, otherwise it must use an exclusive execution
> context which is slow and fragile.  These macros are a legacy of
> the old MP style, and perhaps should just be removed so that new
> applications use locks.

Indeed, these macros are present primarily to make it easier to use legacy
code with the Portable Threads package.  That said, they do provide a layer
that allows the best strategy on a CL implementation to be used for very
brief "atomic" operations (via a lock on some CLs, scheduler controls on
others, etc.).

> without-lock-held:  The SCL reports the whostate only when waiting
> to acquire the lock.  Consider adding a note that this should not
> be used on recursively held locks for which it will be ineffective.

The semantics of WITHOUT-LOCK-HELD with recursive locks needs to be clarified.

> kill-thread: killing a running thread is in general unsafe.
> Consider marking this function as unsafe, and advising developers
> to use a cooperative system for thread exit such as checking a
> flag at safe points.

Indeed.  KILL-THREAD is essentially a debugging/development-time cleanup
tool, and this should be noted in its documentation.

> run-in-thread: interrupting another thread and running arbitrary
> code is in general unsafe.  The other thread could be holding an
> important lock and deadlock.  Consider marking this function as
> unsafe and not recommended for deployed applications.

This is another "strong rope" function, to be used sparingly and with eyes
wide open...

> throwable-sleep-forever, awaken-throwable-sleeper, hibernate-thread,
> awaken-thread: unsafe, deployed applications should not be using
> these, and I suggest just removing them.

The first two functions are internal to Portable Threads.  Hibernate and
awaken provide a useful pattern that has restrictions in the Portable
Interface versions of these functions that avoid problems in the more
general MP versions.  A thread may only cooperatively hibernate itself,
which means it does not return from this form until awakened by another
thread.  While "hibernated," however, the thread will still handle
run-in-thread type operations from another thread.  More "rope," I guess...

> make-condition-variable: should also accept a name for the condition
> variable and also the name for the lock.

Since the Portable Threads unifies the OS-level condition variable and its
associated lock, they should probably not have different names. However,
giving the lock the same name as given the condition variable would be useful.

> condition-variable-wait, condition-variable-wait-with-timeout:
> should accept a whostate to help developers.

A good suggestion!

> condition-variable-signal, condition-variable-broadcast: should not
> require the lock to be held.  The safety is application dependent and
> the broadcast can always be moved outside the lock.

Although POSIX allows these to be called by a thread that does not hold the
mutex, predictable scheduling requires the mutex to be held.  The Portable
Threads condition-variable model enforces this requirement.

> scheduled-function: This seems to be portable code built on the threads
> library which may be better placed elsewhere.

The scheduled functions and periodic functions entities are built on top of
the rest of the Portable Threads interface and technically they could have
been packaged separately (and early on in GBBopen, they once were
separate).  The amount of code for them is small, and the convenience and
simplicity to users of having these included out-of-the-box as part of
Portable Threads (as part of the single portable-threads.lisp file rather
than as separate modules and files) was chosen over having the Portable
Threads package to be a minimal building interface layer for use in
constructing useful thread mechanisms.

----------

Again, thank you for the thoughtful comments & suggestions.  I've tried to
provide some brief explanation/background on the decisions that have been
made thus far with Portable Threads.  The floor is now open...  :-)

-- Dan


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.