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);
--------------------------------------------------------------------------------
  • Add Memory
  • Share This Entry
(0 comments)

Post a comment in response:

This account has disabled anonymous posting.
If you don't have an account you can create one now.
HTML doesn't work in the subject.
More info about formatting

If you are unable to use this captcha for any reason, please contact us by email at support@dreamwidth.org

 
Notice: This account is set to log the IP addresses of everyone who comments.
Links will be displayed as unclickable URLs to help prevent spam.