azure-contrib / AzureDirectory

A Lucene Directory Provider for Azure Blob Storage
Microsoft Public License
77 stars 57 forks source link

Get rid of exceptions that are thrown during normal operation #8

Closed Piedone closed 10 years ago

Piedone commented 10 years ago

First of all, thank you for taking care of the project!

I see that there are many exceptions in normal operation too. There are many related to Blob handling when creating and index, however more problematic, as happening far more is that BlobMutexManager operates with exceptions too. It causes about 6 exceptions for running a search query.

Couldn't this class be modified to use Mutex.TryOpenExisting() instead?

Thank you.

richorama commented 10 years ago

I agree with your suggestion. The TryOpenExisting() would probably be faster. One thing what worries me (and this is probably because I am not an expert on the Mutex class) is that different exceptions are handled in different ways (see https://github.com/richorama/AzureDirectory/blob/master/AzureDirectory/BlobMutexManager.cs#L17)

How would you handle this with your suggested approach?

Happy to accept a PR on this.

Piedone commented 10 years ago

I'm not particularly sure about this either. Will take a look if I can fix it.

Piedone commented 10 years ago

Interestingly enough the MSDN sample uses the same pattern: http://msdn.microsoft.com/en-us/library/c41ybyt3%28v=vs.110%29.aspx

I've come up with the following solution, however I'm not sure if it's correct:

        public static Mutex GrabMutex(string name)
        {
            var mutexName = "luceneSegmentMutex_" + name;

            Mutex mutex;
            var notExisting = false;

            if (Mutex.TryOpenExisting(mutexName, MutexRights.Synchronize | MutexRights.Modify, out mutex))
            {
                return mutex;
            }

            // Here we know the mutex either doesn't exist or we don't have the necessary permissions.

            if (!Mutex.TryOpenExisting(mutexName, MutexRights.ReadPermissions | MutexRights.ChangePermissions, out mutex))
            {
                notExisting = true;
            }

            if (notExisting)
            {
                var worldSid = new SecurityIdentifier(WellKnownSidType.WorldSid, null);
                var security = new MutexSecurity();
                var rule = new MutexAccessRule(worldSid, MutexRights.FullControl, AccessControlType.Allow);
                security.AddAccessRule(rule);
                var mutexIsNew = false;
                return new Mutex(false, mutexName, out mutexIsNew, security);
            }
            else
            {
                var m = Mutex.OpenExisting(mutexName, MutexRights.ReadPermissions | MutexRights.ChangePermissions);
                var security = m.GetAccessControl();
                var user = Environment.UserDomainName + "\\" + Environment.UserName;
                var rule = new MutexAccessRule(user, MutexRights.Synchronize | MutexRights.Modify, AccessControlType.Allow);
                security.AddAccessRule(rule);
                m.SetAccessControl(security);

                return Mutex.OpenExisting(mutexName);
            }
        }

This seems to be working as expected. I'm really not sure however so I posted the question on SO: http://stackoverflow.com/questions/21318156/check-existence-of-mutex-with-correct-permissions-without-exceptions

richorama commented 10 years ago

Great, let's see if anyone answers the SO question.

I guess it would be useful to have some unit tests....

Piedone commented 10 years ago

I think you're more familiar with the project to write unit tests ;-). Joke aside, I've no idea how this could be tested. I think it would need to spawn off multiple threads, or even multiple processes to test if the Mutex works.

richorama commented 10 years ago

ah well, it was worth a try :¬)

I'll try your suggested changes, and run up some tests. Probably next week when I get the time.

Piedone commented 10 years ago

Thank you. I hope in the mean time someone will relate on SO too.

richorama commented 10 years ago

Done!

Thanks for your contribution.

Piedone commented 10 years ago

Thanks!