fu: Close-up of Fu, bringing a scoop of water to her mouth (Default)
fu ([personal profile] fu) wrote in [site community profile] changelog2011-12-14 04:23 pm
  • Add Memory
  • Share This Entry

[dw-free] interest counts not being recorded properly

[commit: http://hg.dwscoalition.org/dw-free/rev/738d19fa977d]

http://bugs.dwscoalition.org/show_bug.cgi?id=1220

Don't pass in $old into set_interests (to avoid the chance it's out of
date); lock around the entirety of set_interests and interest_update to
avoid race conditions. We also now get the current list of intids straight
from the DB, and make sure that the interest exists (or doesn't) -- so we
don't do any work on the DB if we shouldn't need to.

Patch by [personal profile] exor674.

Files modified:
  • cgi-bin/DW/Worker/ContentImporter/Local/Bio.pm
  • cgi-bin/LJ/Keywords.pm
  • cgi-bin/LJ/Setting/Interests.pm
  • cgi-bin/LJ/Widget/CreateAccountProfile.pm
  • htdocs/manage/profile/index.bml
--------------------------------------------------------------------------------
diff -r ee6ce2cebd46 -r 738d19fa977d cgi-bin/DW/Worker/ContentImporter/Local/Bio.pm
--- a/cgi-bin/DW/Worker/ContentImporter/Local/Bio.pm	Thu Dec 15 00:09:14 2011 +0800
+++ b/cgi-bin/DW/Worker/ContentImporter/Local/Bio.pm	Thu Dec 15 00:24:37 2011 +0800
@@ -45,7 +45,7 @@
         push @all_ints, lc( $int ) unless defined( $old_interests->{$int} );
     }
 
-    $u->set_interests( $old_interests, \@all_ints );
+    $u->set_interests( \@all_ints );
 }
 
 =head2 C<< $class->merge_bio_items( $user, $hashref, $items ) >>
diff -r ee6ce2cebd46 -r 738d19fa977d cgi-bin/LJ/Keywords.pm
--- a/cgi-bin/LJ/Keywords.pm	Thu Dec 15 00:09:14 2011 +0800
+++ b/cgi-bin/LJ/Keywords.pm	Thu Dec 15 00:24:37 2011 +0800
@@ -302,6 +302,23 @@
     my ( $add, $del ) = ( $opts{add}, $opts{del} );
     return 1 unless $add || $del;  # nothing to do
 
+    my $lock;
+    unless ( $opts{has_lock} ) {
+        while ( 1 ) {
+            $lock = LJ::locker()->trylock( 'interests:' . $u->userid );
+            last if $lock;
+
+            # pause for 0.0-0.3 seconds to shuffle things up.  generally good behavior
+            # when you're contending for locks.
+            select undef, undef, undef, rand() * 0.3;
+        }
+    }
+
+    my %wanted_add = map { $_ => 1 } @$add;
+    my %wanted_del = map { $_ => 1 } @$del;
+
+    my %cur_ids = map { $_ => 1 } @{ $u->get_interests( { justids => 1, forceids => 1 } ) };
+
     # track if we made changes to refresh memcache later.
     my $did_mod = 0;
 
@@ -311,26 +328,29 @@
     my $uid = $u->userid;
     my $dbh = LJ::get_db_writer() or die LJ::Lang::ml( "error.nodb" );
 
-    if ( $del && @$del ) {
+    my @filtered_del = grep { delete $cur_ids{$_} && ! $wanted_add{$_} } @$del;
+    if ( $del && @filtered_del ) {
         $did_mod = 1;
-        my $intid_in = join ',', map { $dbh->quote( $_ ) } @$del;
+        my $intid_in = join ',', map { $dbh->quote( $_ ) } @filtered_del;
+
         $dbh->do( "DELETE FROM $uitable WHERE userid=$uid AND intid IN ($intid_in)" );
         die $dbh->errstr if $dbh->err;
         $dbh->do( "UPDATE interests SET intcount=intcount-1 WHERE intid IN ($intid_in)" );
         die $dbh->errstr if $dbh->err;
     }
 
-    if ( $add && @$add ) {
+    my @filtered_add = grep { ! $cur_ids{$_}++ && ! $wanted_del{$_} } @$add;
+    if ( $add && @filtered_add ) {
         # assume we've already checked maxinterests
         $did_mod = 1;
-        my $intid_in = join ',', map { $dbh->quote( $_ ) } @$add;
-        my $sqlp = join ',', map { "(?,?)" } @$add;
+        my $intid_in = join ',', map { $dbh->quote( $_ ) } @filtered_add;
+        my $sqlp = join ',', map { "(?,?)" } @filtered_add;
         $dbh->do( "REPLACE INTO $uitable (userid, intid) VALUES $sqlp",
-                  undef, map { ( $uid, $_ ) } @$add );
+                  undef, map { ( $uid, $_ ) } @filtered_add );
         die $dbh->errstr if $dbh->err;
         # set a zero intcount for any new ints
         $dbh->do( "INSERT IGNORE INTO interests (intid, intcount) VALUES $sqlp",
-                  undef, map { ( $_, 0 ) } @$add );
+                  undef, map { ( $_, 0 ) } @filtered_add );
         die $dbh->errstr if $dbh->err;
         # now do the increment for all ints
         $dbh->do( "UPDATE interests SET intcount=intcount+1 WHERE intid IN ($intid_in)" );
@@ -385,17 +405,26 @@
 
 
 # des: Change a user's interests.
-# des-old: hashref of old interests (hashing being interest => intid)
 # des-new: listref of new interests
 # returns: 1 on success, undef on failure
 sub set_interests {
-    my ($u, $old, $new) = @_;
+    my ($u, $new) = @_;
 
     $u = LJ::want_user( $u ) or return undef;
 
-    return undef unless ref $old eq 'HASH';
     return undef unless ref $new eq 'ARRAY';
 
+    my $lock;
+    while ( 1 ) {
+        $lock = LJ::locker()->trylock( 'interests:' . $u->userid );
+        last if $lock;
+
+        # pause for 0.0-0.3 seconds to shuffle things up.  generally good behavior
+        # when you're contending for locks.
+        select undef, undef, undef, rand() * 0.3;
+    }
+
+    my $old = $u->interests( { forceids => 1 } );
     my %int_add = ();
     my %int_del = %$old;  # assume deleting everything, unless in @$new
 
@@ -413,7 +442,7 @@
     }
 
     # Note this does NOT check against maxinterests, do that in the caller.
-    $u->interest_update( add => \@new_intids, del => [ values %int_del ] );
+    $u->interest_update( add => \@new_intids, del => [ values %int_del ], has_lock => 1 );
 
     LJ::Hooks::run_hooks("set_interests", $u, \%int_del, \@new_intids); # interest => intid
 
diff -r ee6ce2cebd46 -r 738d19fa977d cgi-bin/LJ/Setting/Interests.pm
--- a/cgi-bin/LJ/Setting/Interests.pm	Thu Dec 15 00:09:14 2011 +0800
+++ b/cgi-bin/LJ/Setting/Interests.pm	Thu Dec 15 00:24:37 2011 +0800
@@ -72,9 +72,8 @@
 
     my $interest_list = $class->get_arg($args, "interests");
     my @new_interests = LJ::interest_string_to_list($interest_list);
-    my $old_interests = $u->interests;
 
-    $u->set_interests($old_interests, \@new_interests);
+    $u->set_interests( \@new_interests );
 }
 
 1;
diff -r ee6ce2cebd46 -r 738d19fa977d cgi-bin/LJ/Widget/CreateAccountProfile.pm
--- a/cgi-bin/LJ/Widget/CreateAccountProfile.pm	Thu Dec 15 00:09:14 2011 +0800
+++ b/cgi-bin/LJ/Widget/CreateAccountProfile.pm	Thu Dec 15 00:24:37 2011 +0800
@@ -222,8 +222,6 @@
                                               'int' => $err->[1]{int} } );
     }
 
-    my $old_interests = $u->interests;
-
     # bio
     $from_post{errors}->{bio} = LJ::Lang::ml('/manage/profile/index.bml.error.bio.toolong') if length $post->{bio} >= LJ::BMAX_BIO;
     LJ::EmbedModule->parse_module_embed($u, \$post->{bio});
@@ -232,7 +230,7 @@
         $u->update_self( { name => $post->{name} } );
         $u->invalidate_directory_record;
         $u->set_prop('gender', $post->{gender});
-        $u->set_interests($old_interests, \@valid_ints);
+        $u->set_interests( \@valid_ints );
         $u->set_bio($post->{bio}, $post->{bio_absent});
     }
 
diff -r ee6ce2cebd46 -r 738d19fa977d htdocs/manage/profile/index.bml
--- a/htdocs/manage/profile/index.bml	Thu Dec 15 00:09:14 2011 +0800
+++ b/htdocs/manage/profile/index.bml	Thu Dec 15 00:24:37 2011 +0800
@@ -735,7 +735,7 @@
                 return LJ::bad_input( map { BML::ml(@$_) } @interrors );
             }
 
-            $u->set_interests( \%interests, \@valid_ints );
+            $u->set_interests( \@valid_ints );
         }
 
         LJ::Hooks::run_hooks('profile_save', $u);
--------------------------------------------------------------------------------