Race condition in sess_write() causes duplicate entry error in {sessions} table

I get this error several times a day. Because of the trigger_error() call in _db_query(), the error is written to the Apache error log and shows up in our log analyzer.

This patch for Drupal 5 just mutes any error from db_query("INSERT INTO ...") and in case of an error tries an UPDATE instead. What do you think of this?

An alternative approach is to use REPLACE INTO instead on MySQL and something similar on PostgreSQL, but I guess that makes the code more complicated with little gain. I may even be faster to just try the INSERT and then retry with an UPDATE in the rare occasion that the INSERT fails.

Here is a patch for Drupal 6.

BTW I don't know why the trigger_error() call shows up in my Apache error log. It should have been caught by Drupal's error handler, but apparently this doesn't happen. Whatever happens is probably not relevant for this issue, though.

We put it on our production Drupal 5 site yesterday. So far we haven't seen any problems. I'll be out of town the next few days, but I'll get back with an updated status report on Monday.

You are right that the function could be optimized even further. It may be a good idea to completely eliminate the UPDATE {sessions} unless $value had actually changed or at most every X minutes has passed since the session was last written, similar to the session_write_interval check that was introduced in Drupal 6 for UPDATE {users}.

I suggest that we start out with a simple patch that can be applied to Drupal 5, 6, and 7 and then investigate further optimization for Drupal 7 in a separate issue. I don't mind giving it a shot.

So far there has been no problems.

I am new to Drupal development, so I'd like to hear your suggestion on how to proceed.

subscribing. I get these duplicates and also others for the following tables:
- node
- history
- cache_menu

See http://drupal.org/node/230029 re. node table duplicates

Patch updated for HEAD.

The patch is on the right tracks, but we have concurrency issues at more places in the Drupal code base, and the solution used elsewhere is to do first an UPDATE and if it fails an INSERT. This is better because it is more common for the value to be present than absent. From variable_set(), for example:

  db_query("UPDATE {variable} SET value = '%s' WHERE name = '%s'", $serialized_value, $name);
  if (!db_affected_rows()) {
    @db_query("INSERT INTO {variable} (name, value) VALUES ('%s', '%s')", $name, $serialized_value);
  }

I would like to see that solution applied in session.inc, because its cleaner, more easily maintainable (we know the solution works because it is used elsewhere), and more efficient it the common case.

Also, it is common here that bugs get corrected in the current development version (7.x), and then only backported.

Patch updated to use the first-UPDATE-then-INSERT pattern. I also removed the redundant "if" clause (see comment 4).

One possible optimization is to only do the UPDATE if one of the values had changed since the user was loaded from the database in sess_read(), or if session_write_interval has passed since session.timestamp was last updated. This would reduce the number of UPDATE queries quite a bit. Would you prefer this to be discussed in a separate issue?

Putting this into "need review" mode.

The patch looks very sane.

@c960657: you can't really know if one of the value changed, except by saving the initial values. I'm not sure it really worth it.

FWIW, a similar suggestion was discussed (and rejected) in #40545: Improve speed by avoiding unnecessary updates in sess_write().

For me this patch is required, I've made it for 5.7 - every day I'm receiving this error (cased by imagecache probably, but it is sess_write() which is incorrect). Please include in D 5.8 too.

I've seen this on a 4.7 site with high volume, where it cause some really ugly stuff like profile fields for the anonymous user getting values for some other user.

This will perform worse when the site is indexed by crawlers since you are doing a write query there. The patch removes the optimization for requests where no cookie is presented.

@moshe: the behavior didn't change.

What keeps the crawlers out of the session table is the check:

if (!session_save_session() || (empty($_COOKIE[session_name()]) && empty($value))) { (condition 1)
  return TRUE;
}

The logic that was below in the code is actually flawed:

if ($user->uid || $value || count($_COOKIE)) { (condition 2)
  [ insert the cookie ]
}

When we are here, we already passed condition 1, so we have either a not empty $value OR at least a cookie (!empty($_COOKIE[session_name()]). So condition 2 is always true, and thus pretty useless.

Back to code need review. Do you have any other comments Moshe?

Updated Drupal 6 backport of sess_write-drupal7_2.patch.

This looks ready to go in. Does the patch from d7.x still apply (I don't have access to my test system from here)?

Yes, the D7 patch still applies.

I'm not particularly fond of the @-trick. We try to avoid that at all costs because it is not an elegant way to do things.

@Dries: as I explained in #10, this is currently our standard way of dealing with concurrency issues in the MERGE pattern.

From cache_set():

  db_query("UPDATE {" . $table . "} SET data = %b, created = %d, expire = %d, he
aders = '%s', serialized = %d WHERE cid = '%s'", $data, $created, $expire, $head
ers, $serialized, $cid);
  if (!db_affected_rows()) {
    @db_query("INSERT INTO {" . $table . "} (cid, data, created, expire, headers, serialized) VALUES ('%s', %b, %d, %d, '%s', %d)", $cid, $data, $created, $expire, $headers, $serialized);
  }

From variable_set():

    db_query("UPDATE {variable} SET value = '%s' WHERE name = '%s'", $serialized
_value, $name);
    if (!db_affected_rows()) {
      @db_query("INSERT INTO {variable} (name, value) VALUES ('%s', '%s')", $name, $serialized_value);
    }

The DB:TNG will have a merge() operation that is specifically meant to do this in a clever way. In the meantime, this should be committed as soon as possible, at least to D6 (which, without this is still not ready for primetime...).

OK, I can't think of a better solution for now so I'll swallow the @. Committed to CVS HEAD. Thanks for your comments, Damien. Much appreciated.

The 6.x patch in #18 looks ready.

A closely related patch to this one is at #293612: user_authenticate() should work when $_COOKIE is empty, which puts the $user->uid == 0 logic back where it belongs, in the remaining test for crawlers. It would be great if the patch at #293612: user_authenticate() should work when $_COOKIE is empty could be applied just after the patch here has been applied.

subscribing

Subscribing via #52662

It's very interesting, but did it commit into the 6.x branch?

However, I don't think it actually solves the race condition problem.

The problem is originally because there could be race condition between the first SELECT query and the second INSERT query.

Now, same race condition can happen between the UPDATE query and the INSERT query. There will still be an error happening when the record is not available at the time of UPDATE but already exists at the time of INSERT. Of course, we hide the PHP error message by using @. But to me it's not acceptable, because I have a heavy load site and doing MySQL Master-Master replication, once there's an error in the sessions table, the replication was stopped.

What I'm suggesting is to use the

INSERT...ON DUPLICATE KEY UPDATE

syntax.

http://dev.mysql.com/doc/refman/5.0/en/insert.html

    if ($user->uid || $value || count($_COOKIE)) {
      db_query("INSERT INTO {sessions} (sid, uid, cache, hostname, session, timestamp) VALUES ('%s', %d, %d, '%s', '%s', %d) ON DUPLICATE KEY UPDATE uid = %d, cache = %d, hostname = '%s', session = '%s', timestamp = %d ", $key, $user->uid, isset($user->cache) ? $user->cache : '', ip_address(), $value, time(), $user->uid, isset($user->cache) ? $user->cache : '', ip_address(), $value, time());
    }

@yang_yi_cn: INSERT ... ON DUPLICATE is a MySQL-only construct. On Drupal 7, this will be a MERGE, but for Drupal 6 there are no cleaner ways to do this while maintaining cross-database portability.

#18 is still ready to be committeed on D6.

Committed to 6.x, thanks!

Note that this problem has reoccurred in HEAD. This regression is being tracked in #297860: Regression: Convert session.inc to the new database API (and _sess_write should use a merge).

This is a backport of the D6 patch. It includes the D5 backport in #293612: user_authenticate() should work when $_COOKIE is empty which is RTBC, and a backport of the part of the part of #130971: The kitchen sink (remove notices, fix bugs, enhance code styling) that is related to session.inc.

When reviewing this, you may want to apply the patch and diff against the DRUPAL-6 branch. The only differences in sess_write should be the fixes for #142773: Drupal does not fully work when using a reverse proxy and the fix for #40545: Improve speed by avoiding unnecessary updates in sess_write() that have not been backported to D5.

Reroll of D5 patch (the backport of #293612: user_authenticate() should work when $_COOKIE is empty was just committed to the D5 branch).

I did commit this to 5.x, with a bad commit message. I did a another trivial change to get the credit in the CVS log.