PR-staging

Success

User tests: Successful: Unsuccessful:

avatar jaworskimatt
jaworskimatt
20 Nov 2014

See http://issues.joomla.org/tracker/joomla-cms/5138

If JTable::store() happens on the same user concurrently, it will eventually lead to loss of #__user_usergroup_map rows for that user. Because JTable::store() deletes everything from #__user_usergroup_map and then re-adds one by one, if that process happens multiple times in parallel for the same user, the inserts and deletes get mixed up.

This is a fix for this, that instead of performing "delete and re-insert one by one", grabs the records from the database, loops through them, decides which should stay and which should go based on $this->groups and then adds new ones (if any) from $this->groups

avatar jaworskimatt jaworskimatt - open - 20 Nov 2014
avatar jissues-bot jissues-bot - change - 20 Nov 2014
Labels Added: PR-staging
avatar Bakual
Bakual - comment - 20 Nov 2014

Please make sure you follow our codestyle rules. Travis fails because of that. You can see the result when you follow the "Details" link after the Travis message.
The main issue is that we use tabs instead of spaces to indent lines.
Also we use if clauses like this:

if (somescheck)
{
    some code
}
avatar jaworskimatt
jaworskimatt - comment - 20 Nov 2014

Okay:

  • fixed "if" statements to meet the standards
  • used tabs instead of spaces
  • all comments now start with capital letter

Hope this will do

avatar jaworskimatt
jaworskimatt - comment - 20 Nov 2014

Missed (hopefully) one more thing... let's see

avatar Bakual
Bakual - comment - 20 Nov 2014

Travis is fine now. Thanks!
Ready for testing :+1:

avatar Hackwar
Hackwar - comment - 22 Nov 2014

First I would have thought that it would have been easier to simply wrap this in a transaction, but that was not correct since transactions wont help here.

But I think it is wrong that we don't support adding several rows with one query. I would rather fix that issue in our database driver instead of this rather complex change. Fixing that part would be the better fix in my view for this issue, since with one INSERT, we would not have this concurrency issue.

Did you happen on this bug specifically yourself, @jaworskimatt

avatar jaworskimatt
jaworskimatt - comment - 24 Nov 2014

@Hackwar we encountered this bug because multiple (dozens) Jomsocial customers were complaining about user groups getting randomly lost, with some of them the discussions got pretty heated because the bug was first noticed a few months ago. Since Jomsocial saves the user object a lot (don't ask me why, I don't know myself) we got these concurrency issues pretty often. It took us quite a while to track down what was going on (because first we looked for the problem inside Jomsocial itself).

I was thinking about a single insert and/or transactions, but I I am not sure how various database engines would handle them.

avatar rdeutz
rdeutz - comment - 5 Jan 2015

I think it is way better to read/compare and then insert/delete then delete everything and insert. Most of the time there isn't any change in the group member ship so nothing happened at all. Transactions or locking would be the best solution but this here is a step to make it better. Maybe the joomsocial guys should also check if they really need to change the user object every millisecond.

avatar Hackwar
Hackwar - comment - 12 Feb 2015

Please see #6065 for a better solution.

avatar pulsarinformatique
pulsarinformatique - comment - 13 Mar 2015

Hi
It seems the bug remains on J3.4. We just encountered it


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5144.
avatar pulsarinformatique pulsarinformatique - test_item - 13 Mar 2015 - Tested unsuccessfully
avatar izharaazmi
izharaazmi - comment - 21 Jan 2016

Well, long forgotten thread.

I see this PR unsets the values from $this->groups that I think is not appropriate. Am I correct?

avatar brianteeman brianteeman - change - 14 Mar 2016
Category Libraries
avatar brianteeman brianteeman - change - 8 May 2016
Status Pending Needs Review
avatar roland-d roland-d - change - 8 May 2016
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2016-05-08 12:46:55
Closed_By roland-d
avatar roland-d roland-d - close - 8 May 2016

Add a Comment

Login with GitHub to post a comment