two people editing a node at the same time can result in loss of first changes

unless absolutely necessary to serve a cached page, the lock.inc should be included from common.inc, not bootstrap.inc.

Dries: it boils down to, how badly do we need it?

Sounds like we need to post some use cases here. I'll start:

The forum block, the taxonomy_dhtml blocks, and more rely on the drupal cache API to store their data structures. They do so because these structures are expensive to generate, in terms of server resources.

When a user posts a new node, these cached structures are deleted. No problem so far. The problem happens when the next several users request a page. They might *simultaneously race* to become the user who generates the blocks and inserts into the cache table. This can spike the server in a bad way.

This patch enables forum and taxonomy_dhtml to add a lock such that only 1 user at a time tries to generate the block.

As a bonus, this patch comes with nice PHPDoc comments in lock.inc

Locale cache (in core) and filtercache (contrib module) is also involved. We use these two caches as well on our site, and have seen ugly duplicate cache storage requests by the caching code...

I think this patch should be included. I occassionally see mysql errors due to the fact that Drupal attempts to insert cache for an already present cache ID.

I often see those SQL errors, using a lot of stuff cached: archive, filtercache, locale, etc.

In the last coupld of days I've had 38 such errors. 30 of them related to the cache and 8 to the history table. I am now testing ths lock.inc patch and will see how many errors I get.

The patch for cvs was not neccessary, the 4.4 version applied just fine.

I observe the following:

- I get less (only one) cache insert errors.

-. I still get some history insert errors (| 7752 | user error: Duplicate entry '281-1116' for key 1)

- and: I get lock table insert errors:
| 8194 |user error: Duplicate entry '715778' for key 1 query: INSERT INTO locks VALUES('715778', '0.51026200 1086946323', 15, 1, 1)

The reason the @ operator doesn't work is because drupals error handler simply ignores it. The error handler uses the value of error_reporting only to decide whether or not the error should be printed. There have been patches for this before, but they were never applied for some reason.

Assuming this will be fixed someday, do we really need locking in cache_set? Basically the only change is that the errors now occur in lock.inc instead and that a few more queries are executed. Why not just use @db_query in cache_set?

oth the lock.inc is useful in many other cases, i've been using it for a few months now to prevent race conditions caused by simultaneous cron runs (poormanscron) and it works great, haven't seen a single race condition since.

Ignore that attachment, i didn't even attach it. Must be a project.module bug

Jeremy, sorry for the delay. I have now appied your patch against lock.inc.
I actually did only get very few errors in the last weeks (4) and those might be related to some other error not due to file locking at all..

1) I only saw those erros in the watchdog
2) I didn't see any errors after applying this last patch but this is only a few hours ago.
3) Example:

user error: Duplicate entry '352-1059' for key 1 query: INSERT INTO history (uid, nid, timestamp) VALUES (352, 1059, 1089978066) in /home/vdst.net/www/htdocs/includes/database.mysql.inc on line 97.

I only found three entries in the lock table. The site in question isn't very lively.

-1

I think this is the wrong direction to take Drupal. This adds a lot of complexity and doesn't solve the problem. Locks don't work well for interactive multiuser apps because a poorly behaved client can keep a lock out too long. Allowing locks to time out completely erases their usefulness.

For example:
Alice opens a node for editing (taking out a lock on it)
5 minutes pass, Alice is still writing.
The lock times out
Bob opens a node for editing
Bob saves his changes
Alice saves her changes
Bob's changes are gone.

We should deal with race conditions "lazily," as they happen, rather than explicitly locking resources. We should let the database handle locking. That's what it's good at. MediaWiki (the software used by Wikipedia) does things this way.

What should happen is something more along the lines of:
db_begin_transaction();
// Check if conditions are okay for this update
if ($conditions_ok) {
// run queries
}
else {
// Deal with race condition
}
db_commit_transaction();

Dealing with race conditions for editing a node could be as simple as throwing the user back into the edit screen and complaining that the node has changed since they last edited it. My revisions table patch has implementations of db_begin_transaction() and db_commit_transaction() for both MySQL and PostgreSQL.

That last comment was me. I forgot to log in.

Sorry.

I agree with a few of the others, lets keep this simple. No unnecessary UI or API should be added. That will make everyone's lives easier.

For a specific (not entirely thought out) idea maybe "branching" or "conflicting" versions could be built into the revisions system somehow with appropriate messages and UI flow.

Can you post a comprehensive and updated patch for a more detailed review? The code seems rather scattered now and I can't figure out what exactly to concentrate on reading to make a better review.

Besides node editing, what other use use cases would be possible for lock.inc in core Drupal and contributed modules?

From a users' perspective, I can't see how it makes sense to lock nodes for an arbitrary time period (here, 5 minutes) after somebody clicks, "edit". If two users both want to contribute to the content on a given node, we want them both to be able to contribute. There's no reason to force users to race the clock in submitting content (5 minutes is not that much time for an intelligent contribution). And it's ENTIRELY UNACCEPTABLE to have users lose the content they submit. If people are spending time making contributions to our site's content, and their changes are lost as soon as they submit them, that's going to demoralize the most helpful participants; if you're running a community-supported site, you can't just ask people for help and then make their contributions disappear into the ether. This patch violates both of those principles.

Consider the following situations:

Alice takes 15 minutes to finish an edit to a collaboratively editable part of the site. At 6 minutes, when the file is no longer locked, Bob comes along and makes an edit that takes him 5 minutes to finish. The system lets Bob begin his revision, and at 11 minutes Bob posts his revision. Four minutes later Alice unknowingly makes Bob's revision disappear into space. Not cool.

We could say "you have 5 minutes to submit your revision, or you might zap someone else's revision" but most people will probably just ignore that (after all, *their* work isn't at stake). We could require them to finish their revisions within 5 minutes, but that won't do much for the quality of our content. And say we extend this 5 minutes to another arbitrary--but longer--15 minutes, to give people time to write. Now say Alice starts a revision and gets an important phonecall, Bob wants to edit the file too, because it contains an embarassing error. Unfortunately for Bob, he's unnecessarily locked out for a good 15 minutes.

If your rules are "no potential editor gets turned away" and "nobody's changes get unconsciously destroyed", there's a rather simple solution to this problem: the Wikipedia model. If somebody edits the same page between when you start and finish editing, you get a screen that lets you compare your changes and merge them. Do a little experiment for yourself here: wikipedia.org. The merging screen is not nearly as pretty as it could be, but one could definitely make something better. But the main point is it doesn't violate the two principles "don't reject / demoralize your users" principles.

Bugzilla supports collision detection, without (I presume) locking. While I haven't looked at the code I am guessing that the time is captured when the form is generated and when the form is submitted a check is made to see if the node was updated between the form start time and the form submit time. I have experienced collisions in bugzilla before and the system simply notifies the user that another user updated the node before you and gives you an opportunity to resubmit.

So, why not do table level locking for MySQL 3.x and transactions for databases that support it?

It seems to make more sense than writing our own locking system.

Despite all the chatter in this issue, the final patch is small and useful IMO.

+1 This is a good solution, if only temporary while we may debate more thorough/extensive ways to handle it.

I agree. The concept of this latest patch is good, but as Jeremy said, it still has race conditions.

Specifically, the following scenario could happen in node_submit():
* Alice starts editing the node
* Bob starts editing the node
* Alice starts saving the node.
* Bob starts saving the node.
* Alice's process gets control of the CPU first and passes the line"$node = node_validate($node);" in node_submit()
* Bob's process gets the CPU and completely validates and saves the node
* Alice's process has already passed the validation line, so it continues saving the node, overwriting Bob's changes.

A lock or transaction starting above the $node = node_validate($node); line and ending below node_save() will make sure that the two processes can't enter that region at once and prevent the race condition.

We should fix this before we commit it to CVS. If we don't fix the race conditions issue, then sites with heavy traffic will still lose data.

I'm not sure about that. From an end user point-of-view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often.

I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code.

I'm not sure about that. From an end user point-of-view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often.

I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code.

I'm not sure about that. From an end user point-of-view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often.

I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code.

I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one.

I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code.

I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one.

I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code.

I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one.

I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code.

I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one.

I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code.

I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one.

I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code.

I still say we shouldn't commit known buggy code to CVS, even if it fixes an existing bug.

That's not acceptable.

Committed to HEAD.

Sorry for dredging this closed issue up again, but there is one other small change that would massively reduce frustration caused by this patch: display a warning message when the second person starts to edit the page, telling them that another person is currently working on it.

If you're working on a page where you're making non-trivial changes then you might waste an awful lot of time making the changes all over again when you find another person has slightly altered the content you've started to work on.

It doesn't help the first person, of course, if the second person saves their changes first. But it would be a nice improvement.