Multi-Threading issues

Developer
Apr 17, 2011 at 5:47 AM

I keep finding that during testing my user file is empty after I use multiple browsers to test a secure portion of my site. It seems (after looking through your code) that the membership provider for xml content isn't multi-thread safe. I understand that there are several lock's throughout the code in all the right places, but I think there could be a problem with conditions where there are multiple cores on a machine and multiple threads. I think in my case there are multiple instances of the membership provider being created and they are creating a race condition for the xml file being written and read, thus deleting the users on deserializing of an empty file. I think you might want to consider making it much more multi-core safe by using mutex's as they are meant to lock a resource across all threads regardless of if the object has been created more than once. 

Thanks for looking into this.

-Jon Ross

http://msdn.microsoft.com/en-us/library/aa645740(v=vs.71).aspx#vcwlkthreadingtutorialexample4mutex

Coordinator
Apr 18, 2011 at 7:34 AM

Hi Jonathan,

Thanks for the great tip.
I will for sure no only consider but make it much more multi-core safe.
I will issue a new version as soon as I find some time to work on it.

Best Regards

Developer
Apr 18, 2011 at 2:51 PM

I'm fine with helping out in this matter, I was just blocked by a problem with loading the XmlProviders project and I am not sure if I need to. I need to have this fixed for some projects of mine so I have time to do the change over to mutex's if I can get the project to load correctly. Any idea why I would get this error: Artem.XmlProviders.csproj : error  : The application for the project is not installed.? Thanks.

Developer
Apr 19, 2011 at 4:06 AM

I edited the Persitable class by adding one helper method and edited the Load() and Save() commands. I confirmed that it works and that it waits for one Save/Load to finish before it continues on. 

        /// <summary>
        /// Creates a MD5 hash from a string. Used for creating a unique name for the mutex's for each file that is accessed by the store objects. (putting the
        /// actual file name as a mutex name was resulting in a DirectoryNotFound error, which I believe was because of some special rules the
        /// Mutex name must follow, but aren't well documented. 
        /// </summary>
        /// <param name="strInput">Unhashed string</param>
        /// <returns>Hashed string</returns>
        private static string CalculateMD5Hash(string strInput)
        {
            System.Security.Cryptography.MD5 md5 = System.Security.Cryptography.MD5.Create();
            byte[] inputBytes = Encoding.ASCII.GetBytes(strInput);
            byte[] hash = md5.ComputeHash(inputBytes);

            StringBuilder sb = new StringBuilder();
            for (int i = 0; i < hash.Length; i++)
            {
                sb.Append(hash[i].ToString("x2"));
            }
            return sb.ToString();
        }

        /// <summary>
        /// Loads the data from the filesystem. For deserialization an XmlSeralizer is used.
        /// </summary>
        [SecuritySafeCritical]
        public virtual void Load() {

            string file = this._file;
            Mutex applicationFileLock = null;
            try {
                string fileNameHash = CalculateMD5Hash(_file);
                applicationFileLock = new Mutex(false, fileNameHash );
                applicationFileLock.WaitOne();
                if (System.IO.File.Exists(file))
                {
                    using (FileStream reader = File.Open(file, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) {
                        _value = (T)_serializer.Deserialize(reader);
                    }
                }
                AddFileNotifier();
            }
            catch (Exception ex) {
                throw new Exception(string.Format(Messages.LoadFailed, file), ex);
            }
            finally
            {
                if (applicationFileLock != null)
                {
                    applicationFileLock.ReleaseMutex();
                } 
            }
        }

        /// <summary>
        /// Persists the data back to the filesystem
        /// </summary>
        [SecuritySafeCritical]
        public virtual void Save() {

            if (!this.IsEmpty) {
                string file = this.DirectWrite ? _file : Path.GetTempFileName();
                var value = _value;
                _saving = true;
                Mutex applicationFileLock = null;
                try {
                    string fileNameHash = CalculateMD5Hash(_file);
                    applicationFileLock = new Mutex(false, fileNameHash);
                    applicationFileLock.WaitOne();
                    FileMode mode = File.Exists(file) ? FileMode.Truncate : FileMode.OpenOrCreate;
                    using (FileStream stream = File.Open(file, mode, FileAccess.Write, FileShare.ReadWrite)) {
                        _serializer.Serialize((Stream)stream, value);
                    }

                    if (!this.DirectWrite) {
                        File.Copy(file, _file, true);
                        File.Delete(file);
                    }
                }
                catch (Exception ex) {
                    throw new Exception(string.Format(Messages.SaveFailed, file), ex);
                }
                finally {
                    _saving = false;
                    if (applicationFileLock != null)
                    {
                        applicationFileLock.ReleaseMutex();
                    }
                }
            }
        }

Coordinator
Apr 19, 2011 at 7:06 AM

Hi Jonathan,

Thank you very much for collaborating in the project and providing me the fix code.
I'm going do my best to issue a new release by the end of this week.

Best Regards

Developer
Apr 20, 2011 at 5:59 AM

Humm, it seems to be something else that is producing the empty users file. 

I logged in with one browser this time and right afterward found my user file looking like this

<ArrayOfXmlUser xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xsi:nil="true" />
The only thing that I can think is happening is that the GC is removing the instance before save gets called and then it is an empty/null user list at that point and it saves that data.
I think I will need to test this with a conditional breakpoint to see what's happening. 
What was the reasoning for trying to keep the object out of the Garbage collector? Was it just optimization code or was it to solve some specific problem?

Developer
Apr 20, 2011 at 8:24 AM

I believe I have found other multi-threading issues. In several places on persitable the variable _value is being edited without a lock. This causes a problem if

after thread 1 gets past if(!this.IsEmpty) then thread 2 decides to set _value to default(T) or new T() then returns control to thread 1, by the time we are locking down everything _value equals an empty list. I will have to see how to edit this to make

this whole class safe, not just editing of the file. 

Developer
Apr 21, 2011 at 6:46 AM

 

Fixes needed for XMLMembership

1) The code has several locks where their scope is incorrect. You aren't dealing with a database record or single variable in memory where it would be fine to lock just the current object. I have made SyncRoot static in the provider classes as multiple instances can be created for these objects with seporate versions of the user list possible (i.e. one object deletes a user, but at the same time another user is added to the list in another object, whichever one gets to saving last gets what it was intending. By making it static it will force all threads to block while the store is being edited in any object).

2) You have many code areas that have try/catch blocks that arn't doing anything. This might be a preferred style for you, but they might be providing a false sense of security for other programmers. (optional, and of course, my opinion)

Remove these:

 try
 {
       //code
 }
 catch
 {
      throw;
 }
3) WeakReference _storeRef;
Can't we just let these poor, old, battered, and used classes just die? I don't have much experiance with disposed objects,
 but I feel safer working with the living and not the undead when it comes to objects. 

4) the query in FindUsersInRole isn't filtering by role
replace with:
                var query = from role in this.Roles.AsQueryable()
                            from user in role.Users
                            where (user.IndexOf(usernameToMatch, comparison) >= 0) 
					&& role.Name.Equals(roleName, comparison)
                            select user;

I have some pretty heavily modified code that I have tested and made sure it fixes the problem of User.xml being saved as an empty object. I will post what I have for you to review 
(all these fixes are in there, but I got rid of things you might feel you like to have in there as I feel I will be maintaining the code I modified and might as well have it the way
I like it. But the locking is done correctly for XML stores, but not anything for your extended things like sql server compact).


Coordinator
Apr 21, 2011 at 8:28 AM

Jonathan,

I really appreciate your great effort on XML Providers.
As you can find I have created a new planned release 4.5 for all your fixes and improvements.
You are right about the required changes and we will do them.
Just for the try{ }catch { throw } blocks I thought to implement some nice error handling at some point and had not time for that.
The other reason I had them is to have the actual method name in my code in the exception trace information.
However, I don't mind at all to be removed :).

I would like to add you as a team member and that way to give you access to the project TFS repository, in order if you like to maintain you changes to the project repository and you like to collaborate to project.
So, please let me know if you would like to be added as a project member.

Thanks for the great work and support.

Regards

Developer
Apr 22, 2011 at 4:35 AM

Sure, I have an interest in this project and I have already modified the code to fix the problems for my use and I wouldn't mind sharing those fixes. I wasn't going to ask to be added to the project and was hoping to at least show you what I had done but there isn't a spot on here to upload attachments to this post. I was hoping you would add me as a contributor or at least provide me with an email that I could forward changes to. I have some ideas on how to lock portions of the code for XML and not lock them if you are connecting to a more record based project. 

About the try/catch blocks, I am glad they were just placeholders. I don't mind leaving them, I just would rather see them removed if something wasn't going to be added in the future. 

I am still curious about the WeakReference objects and why they are used and not just re-instantiating the stores if they are gone. I am trying to learn from this code as I don't know everything. I only claim to understand mildly how to work with threads :).  At my day job we do pair programming exclusively so I get code reviews constantly and that is what I think this project has been lacking. It just needed another pair of eyes from a different point of view to expose what has been happing. 

Thanks for the help and for allowing me to join the project.

 

Developer
Apr 22, 2011 at 4:36 AM

Oh, and could I be added to the .net 2.0/3/3.5 project also. I have project that relies on that library also, so I would like to fix it up so it doesn't have the same problems. 

Coordinator
Apr 22, 2011 at 7:49 AM

You have beed added to the both projects.
Wecome to the team :).
Good point about the pair programming - 100% agree.
I will send you my skype name, if you would like to add me and discuss any issues.

Regards

Apr 27, 2011 at 8:55 AM

That's great news. We have also had similar problems with users.xml and are eagerly waiting for the release !

Thanks in advance !

Developer
Apr 29, 2011 at 1:18 AM

I got slowed down by a cold. I will implement my fixes this Saturday.

Nov 1, 2011 at 10:58 AM

I started using this new version and its looking pretty good. Thanks a ton Jonathan and Velio

Best Regards