Document that clock drift will cause lock system to fail

Ooops - 30 seconds. Less likely but still an issue.

Well that's a little tricky and I'm not sure that it's all down to documentation.

There are some assumptions in the code that the time-slop is in the order of milliseconds. Adjustments made by NTP can result in sudden jerks in time several orders of magnitude greater than that. If you are in a hosted environment you have little or no control over time...it's down to the admins at the hosting site I guess.

Given all that I would suggest some words to the effect that time adjustments and lack of close synchronisation between systems will impact the reliability of the locking sub-system in single and multi-server environments. It will not be 100% reliable even if the applications/modules are well coded. Don't try and launch rockets or run pacemakers using it :-)

See if this works...based on drupal-7.0-alpha4 which is what I have downloaded at present. Patch created in NetBeans, I hope I got the files the right way around :-)

If it all goes pear shaped here is the text of the revised comments at the start of the module:

/**
 * @defgroup lock Functions to coordinate long-running operations across requests.
 * @{
 * In most environments, multiple Drupal page requests (a.k.a. threads or
 * processes) will execute in parallel. This leads to potential conflicts or
 * race conditions when two requests execute the same code at the same time. A
 * common example of this is a rebuild like menu_rebuild() where we invoke many
 * hook implementations to get and process data from all active modules, and
 * then delete the current data in the database to insert the new afterwards.
 *
 * This is a cooperative, advisory lock system. Any long-running operation
 * that could potentially be attempted in parallel by multiple requests should
 * try to acquire a lock before proceeding. By obtainng a lock, one request
 * notifies any other requests that a specific operation is in progress which
 * must not be executed in parallel.
 *
 * To use this API, pick a unique name for the lock. A sensible choice is the
 * name of the function performing the operation. 
 *
 * If a function acquires a lock it should always release it when the
 * operation is complete by calling lock_release(), as in the example.
 *
 * A function that has acquired a lock may attempt to renew a lock (extend the
 * duration of the lock) by calling lock_acquire() again during the operation.
 * Failure to renew a lock is indicative that another request has acquired
 * the lock, and that the current operation may need to be aborted.
 *
 * If a function fails to acquire a lock it may either immediately return, or
 * it may call lock_wait() if the rest of the current page request requires
 * that the operation in question be complete. After lock_wait() returns,
 * the function may again attempt to acquire the lock, or may simply allow the
 * page request to proceed on the assumption that a parallel request completed
 * the operation.
 *
 * lock_acquire() and lock_wait() will automatically break (delete) a lock
 * whose duration has exceeded the timeout specified when it was acquired.
 *
 * Alternative implementations of this API (such as APC) may be substituted
 * by setting the 'lock_inc' variable to an alternate include filepath. Since
 * this is an API intended to support alternative implementations, code using
 * this API should never rely upon specific implementation details (for example
 * no code should look for or directly modify a lock in the {semaphore} table).
 *
 * LIMITATIONS:
 * The following limitations in this implementation should be carefully noted:
 *
 * Time - Timestamps are derived from the local system clock of the processor
 *        the code is executing on. The orderly progression of time from this
 *        viewpoint can be disrupted by external events such as NTP
 *        synchronisation and operator intervention. Where multiple web servers
 *        are involved in serving the site they will have their own independent
 *        clocks introducing another source of error in the time keeping process.
 *        Timeout values applied to locks must therefore be considered approximate
 *        and should not be relied upon.
 * Uniqueness of lock names
 *        This is not enforced. The impact of the use of a common lock name will
 *        depend on what processes and resources the lock is being used to manage.
 *        Best case is a denial of service situation.
 * Limited support for resources shared across sites
 *        The locks are stored as rows in the semaphore table and, as such, they
 *        have the same visibility as the table. If managed resources are shared
 *        across sites then the semaphore table must be shared across sites. This
 *        is a binary situation, either all resources are shared and the semaphore
 *        table is shared or no resources are shared and the semaphore table is
 *        not shared. Mixed mode operation is not supported.
 *
 * SIDE EFFECTS:
 * A global array, $locks, is created by the lock management API.
 *
 * EXAMPLES:
 * A very simple example use of this API:
 * @code
 * function mymodule_long_operation() {
 *   if (lock_acquire('mymodule_long_operation')) {
 *     // Do the long operation here.
 *     // ...
 *     lock_release('mymodule_long_operation');
 *   }
 * }
 * @endcode
 *
 */

I will dig a bit deeper. I had assumed, based on this extensive documentation in the patch creation documentation, that NetBeans produced the right format.

Trying again.....

Re global array: What happens if it gets stomped on? This is a warning that if you use the locks API then you'd better not use a global array called locks in your own code. This should be a standard part of the documentation, if the API implementation leaves things hanging around that the user can trip over then document it. If the API also uses particular resources and hangs onto them, document it, etc etc. This avoids nasty surprises.

Regards to wrapping. Is this because the output from the API module doesn't just glob all the lines into a single para or because it looks ugly for those reading the raw code?

I will have one more go and try and fix the end-of-line issue, if that doesn't do it then I'm not going to waste any more time on it.

Replaced CRLF with LF

I think I know why the patch is failing, I don't know if there will be any further obstacles once I get past this one, I'm not going to find out.

If you rely on the code for glossing over deficiencies in the documentation, and cultivate the attitude that it's OK to leave things out because it's all in the code, then your API documentation is not going to get any better.

If you factor virtualisation into the picture it can become even more confusing....and BIOS is very much a PC thing. There will be a hardware clock that the O/S will initially take its time from at boot, it does not have to rely on this clock beyond that point although it does need an interrupt source to maintain its own notion of time. Applications may also maintain an independent view of time that is distinct from the other views of time around them. So it is not simple.

If you want a rewrite I would therefore suggest:

"Timestamps are derived from the local system clock of the environment the code is executing in"

Fundamental flaw = Corner case.

Nice.

===== Edit =====

From the D6 settings.php:

 * To provide prefixes for specific tables, set $db_prefix as an array.
 * The array's keys are the table names and the values are the prefixes.
 * The 'default' element holds the prefix for any tables not specified
 * elsewhere in the array. Example:
 *
 *   $db_prefix = array(
 *     'default'   => 'main_',
 *     'users'     => 'shared_',
 *     'sessions'  => 'shared_',
 *     'role'      => 'shared_',
 *     'authmap'   => 'shared_',
 *   );

This is a documented supported configuration option...but not supported by the lock routine. Feature????? Next you'll be saying that a 'memory leak' isn't a bug ;-)