bugGNUstep - Bugs: bug #25307, [NSConditionLock...

Group
 
 

bug #25307: [NSConditionLock tryLockWhenCondition:] shouldn't throw exception when lock is unavailable

Submitter:  Doug Simons <theeggcamefirst>
Submitted:  Tue 13 Jan 2009 09:32:21 PM UTC
   
 
Category:  Base/Foundation Severity:  3 - Normal
Item Group:  Bug Status:  Ready For Test
Privacy:  Public Assigned to:  gcasa
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 30 Jan 2009 07:37:16 PM UTC, comment #16: 

Tested.

Richard Frith-Macdonald <CaS>
Group Member
Fri 30 Jan 2009 02:35:36 PM UTC, comment #15: 

I will put them in the test suite from now on.

It doesn't make sense to let it continue although, at that point, I'm not sure what difference it makes.   I, however, believe it's safer to do what Mac OS X does here since we don't know how the applications might behave otherwise.

GC

Gregory John Casamento <gcasa>
Group administrator
Fri 30 Jan 2009 12:11:23 PM UTC, comment #14: 

I put the tests in the testsuite, ran it on MacOS-X, and checked the results, then changed the case of NSConditionLock-tryLock in GNUstpe to match the behavior in MacOS-X.

I noticed the comments about MacOS-X deadlocking for NSConnectonLock attempting to lock recursively.  That was already flagged in the testsuite for NSLock as a MacOS-X bug.
However, when I consider it, I don't think the MacOS-X behavior is totally stupid  ... perhaps we should mimic MacOS-X and halt the thread in this situation rather than raising an exception.   Does it make sense to allow an application to deadlock a thread and (perhaps) have other threads continue working?

Richard Frith-Macdonald <CaS>
Group Member
Fri 30 Jan 2009 07:57:19 AM UTC, comment #13: 

Aha ... I found the test program.
I'm attaching them here and will rewrite them and add them to the testsuite.
Please, in future could you put testcases in the testsuite.  Our regression testing will only work if we add testcases when we fix things.

(file #17355)

Richard Frith-Macdonald <CaS>
Group Member
Thu 29 Jan 2009 11:18:19 PM UTC, comment #12: 

I've attached the OpenStep results, in case there's any doubt so that we can have complete documentation.

(file #17354)

Gregory John Casamento <gcasa>
Group administrator
Thu 29 Jan 2009 11:04:31 PM UTC, comment #11: 

The result is the same on OpenStep as on Cocoa as well.

I have committed the change which was previously made back into the repository.  I have made it a warning rather than an exception.

Thanks, GC

Gregory John Casamento <gcasa>
Group administrator
Thu 29 Jan 2009 09:33:45 PM UTC, comment #10: 

So, as you can see, in our case.  The ONLY cases which fail in the single threaded case are those for NSConditionLock.

I believe this sufficiently confirms the observed behavior to reinstate the previous change and to also change NSLock tryLock to behave accordingly.

GC

Gregory John Casamento <gcasa>
Group administrator
Thu 29 Jan 2009 09:32:02 PM UTC, comment #9: 

On Mac OS X:
============
2009-01-29 16:24:05.226 locktest[4344:10b] ======== SINGLE THREADED TESTS
2009-01-29 16:24:05.229 locktest[4344:10b] [NSLock lock] test passed -- Mac OS X behavior is to deadlock in this case, we throw an exception.
2009-01-29 16:24:05.230 locktest[4344:10b] [NSConditionLock lock] test passed -- Mac OS X behavior is to deadlock in this case, we throw an exception.
2009-01-29 16:24:05.230 locktest[4344:10b] [NSRecursiveLock lock] test passed
2009-01-29 16:24:05.231 locktest[4344:10b] [NSLock tryLock] test passed
2009-01-29 16:24:05.232 locktest[4344:10b] [NSConditionLock tryLock] test passed
2009-01-29 16:24:05.233 locktest[4344:10b] [NSRecursiveLock tryLock] test passed
2009-01-29 16:24:05.233 locktest[4344:10b] ======== MULTI THREADED TESTS

NOTE: For the first two cases, I had to make them pass since throwing an exception is basically equivalent to letting them deadlock.  I'm wondering if we should also allow them to deadlock in those cases.

The same code on GNUstep:
=========================
2009-01-29 16:36:35.890 LockTest[30778] ======== SINGLE THREADED TESTS
2009-01-29 16:36:35.891 LockTest[30778] [NSLock lock] test passed
2009-01-29 16:36:35.891 LockTest[30778] [NSConditionLock lock] test failed
2009-01-29 16:36:35.891 LockTest[30778] [NSRecursiveLock lock] test passed
2009-01-29 16:36:35.891 LockTest[30778] [NSLock tryLock] test passed
2009-01-29 16:36:35.891 LockTest[30778] [NSConditionLock tryLock] test failed
2009-01-29 16:36:35.892 LockTest[30778] [NSRecursiveLock tryLock] test passed
2009-01-29 16:36:35.892 LockTest[30778] ======== MULTI THREADED TESTS


Gregory John Casamento <gcasa>
Group administrator
Thu 29 Jan 2009 02:56:17 AM UTC, comment #8: 

The test is called "locktest".

Gregory John Casamento <gcasa>
Group administrator
Thu 29 Jan 2009 02:48:52 AM UTC, comment #7: 

I have implemented and added a test to the Testing directory which shows that tryLock does not throw an exception when locking the same lock twice in the same thread.

I am going to put additional tests into this test case, if necessary.   Currently the code causes the following on GNUstep:

locktest.m:65: main: Assertion 'pass == (BOOL)1' failed.

That does not happen on Mac OS X with the same code.

Gregory John Casamento <gcasa>
Group administrator
Sun 18 Jan 2009 08:57:35 PM UTC, comment #6: 


> > The point of "tryLock" and "tryLockWhenCondition:" is to attempt to do the
> > lock and return NO, if it can't.


> Sure ... but the rationale was that, if the lock is held by the current
> thread then calling it is evidence of a probable bug.


In the code I'm porting it wasn't evidence of a bug, although it might be argued that it was evidence of some laziness in coding. :-)

In any case, I didn't file this bug report because GNUstep's philosophy seemed wrong, but because its behavior was different from Cocoa's. At this point I've cleaned up my code so it no longer tries the lock when it already holds it, but I thought you'd like to know that Cocoa simply returns NO on the "try..." methods if it's not able to obtain the lock (for whatever reason).

Doug Simons <theeggcamefirst>
Sun 18 Jan 2009 07:51:34 AM UTC, comment #5: 


>  I will go ahead and write one and attach it here.


You could add it to tests/testsuite/base/

Richard Frith-Macdonald <CaS>
Group Member
Sun 18 Jan 2009 07:40:11 AM UTC, comment #4: 


> The point of "tryLock" and "tryLockWhenCondition:" is to attempt to do the
> lock and return NO, if it can't.


Sure ... but the rationale was that, if the lock is held by the current thread then calling it is evidence of a probable bug.
Why?  Because if a thread holds a lock it is also responsible for releasing the lock ... which means it must know that it holds the lock ...  in which case there is no need for it to try locking again.
I'm pretty sure that's the rationale behind the current code... though I'm not sure I consider it valid as I can construct scenarios where you could do this and not have a bug (though your code might be hard to understand).

>  For it to return an exception seems wrong in any case.  In my opinion it should
> return NO whether a single thread attempts to lock the same lock twice or not.


I think it should do whatever MacOS-X does ... but at least we should log a warning message if a thread tries to obtain a lock twice.

> I believe what we need is a minimal test case to determine how this behaves
> on Mac OS X.   I will go ahead and write one and attach it here.



Sounds good ... please remember that we should test (and make changes where necessary) all the locking classes, not just NSConditionLock.


Richard Frith-Macdonald <CaS>
Group Member
Sat 17 Jan 2009 05:52:13 PM UTC, comment #3: 

The point of "tryLock" and "tryLockWhenCondition:" is to attempt to do the lock and return NO, if it can't.   For it to return an exception seems wrong in any case.  In my opinion it should return NO whether a single thread attempts to lock the same lock twice or not.

I believe what we need is a minimal test case to determine how this behaves on Mac OS X.   I will go ahead and write one and attach it here.

GC

Gregory John Casamento <gcasa>
Group administrator
Sat 17 Jan 2009 08:40:25 AM UTC, comment #2: 

If this is about the exception thrown if a single thread attempts to lock the same lock twice, it's not a behavior specific to NSConditionLock (only an NSRecursiveLock is allowed to be locked twice by a thread).

If the exception is something else, please disregard the rest of this comment...

The Cocoa documentation actually says nothing about exceptions in this case, but  I recall there was some discussion about lock behavior a few years ago, which concluded that the GNUstep behavior was the best available ... it returns NO if the lock is unavailable, but raises an exception if you try to lock the lock twice from the same thread, since locking twice from the same thread is almost guaranteed to be a programming error.

If we want to remove this check, we need to do it consistently for all locks and document the new behavior.
Perhaps we could change to logging a warning instead of raising an exception (and/or restricting the behavior to the debug version of the library) ... once we have tested MacOS-X behavior with a test case.

Richard Frith-Macdonald <CaS>
Group Member
Fri 16 Jan 2009 09:29:26 PM UTC, comment #1: 

What's the exception that's being thrown?

Gregory John Casamento <gcasa>
Group administrator
Tue 13 Jan 2009 09:32:21 PM UTC, original submission:  

The tryLockWhenCondition: method of NSConditionLock throws an exception when the lock is already locked. Cocoa simply returns NO in this situation.


Doug Simons <theeggcamefirst>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #17355:  locktest.tar.gz added by CaS (5KiB - application/x-gzip)
file #17354:  OpenStep_test.rtf added by gcasa (2KiB - application/rtf - OpenStep results.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by CaS (Posted a comment)
  • -email is unavailable- added by gcasa (Posted a comment)
  • -email is unavailable- added by theeggcamefirst (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-01-30 CaS Open/ClosedIn Test Closed
    2009-01-30 CaS Attached File- Added locktest.tar.gz, #17355
    2009-01-29 gcasa Attached File- Added OpenStep_test.rtf, #17354
    2009-01-29 gcasa StatusIn Progress Ready For Test
        Open/ClosedOpen In Test
    2009-01-29 gcasa StatusReady For Test In Progress
        Open/ClosedIn Test Open
    2009-01-16 gcasa Assigned toNone gcasa
        Open/ClosedOpen In Test
    2009-01-16 gcasa StatusNone Ready For Test

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code