bugGNUstep - Bugs: bug #59204, NSOperationQueue leak

 
 

bug #59204: NSOperationQueue leak

Submitted by:  Larry Campbell <lcampbel>
Submitted on:  Tue 29 Sep 2020 12:39:15 PM UTC  
 
Category:  Base/Foundation Severity:  3 - Normal
Item Group:  Bug Status:  Ready For Test
Privacy:  Public Assigned to:  Richard Frith-Macdonald <CaS>
Open/Closed:  In Test

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Wed 14 Oct 2020 08:13:32 AM UTC, comment #14: 

I changed the definition of the macros in trunk (and added some comments).  I do worry that there might be code that's broken by the behavior change though (I found/fixed one place).

Richard Frith-Macdonald <CaS>
Project MemberIn charge of this item.
Mon 12 Oct 2020 08:28:57 AM UTC, comment #13: 

I think we all agree that the loop in the macros is more trouble than it's worth, and we should change it.

> Since you often want to wrap the body of a loop in an
> autorelease pool, hijacking break & continue is
> particularly dangerous.


Actually it's quite common practice to empty an autorelease pool every N times round a loop, so that you don't have the computational overhead of emptying a pool in each loop iteration, which can be very inefficient.  The @autorelease{ ...} construct in a loop does not permit that (the one area I can think of in which ARC code is less efficient or more cumbersome than pre-arc code).

Richard Frith-Macdonald <CaS>
Project MemberIn charge of this item.
Mon 12 Oct 2020 07:20:40 AM UTC, comment #12: 

You are right, even in the non-ARC version of the macro the loop does more harm. I think it is there to allow the draining of the pool, but this does break the expected behaviour of the break statement. This means "break" and "return" will leak in that case.

Fred Kiefer <FredKiefer>
Project Member
Sun 11 Oct 2020 03:01:17 PM UTC, comment #11: 

I don't see why we need to support 'break' at all. In other words, I propose getting rid of the do/while loop in both cases.

Since you often want to wrap the body of a loop in an autorelease pool, hijacking break & continue is particularly dangerous.

And yes, there's no good way to handle return. Other than by commenting the macros and making sure people are aware of the difficulty.

Larry Campbell <lcampbel>
Sun 11 Oct 2020 12:26:19 PM UTC, comment #10: 

My impression is that we should change the ARC version of this macro to not include the do/while loop. As for the non-ARC version, there we need the loop to handle "break" correctly. But there is no way to handle the case of a return statement in the loop. We really need to check all the usages in the GNUstep code whether they are affected.

Also the documentation of these macros needs to be improved to explain the limits in more detail.

And in this specific case where there is already a "break" in the code these macros are just not directly usable.

Fred Kiefer <FredKiefer>
Project Member
Sun 11 Oct 2020 10:27:54 AM UTC, comment #9: 

I altered the retain/release change.

The ENTER_POOL/LEAVE_POOL macros are presumably intended to marry the original retain/release mechanism with ARC semantics, and don't really work.  I wonder if they are perhaps more of a danger than an improvement.

Richard Frith-Macdonald <CaS>
Project MemberIn charge of this item.
Sat 10 Oct 2020 07:51:12 PM UTC, comment #8: 

oops, pasted too fast; obviously I meant to suggest that ENTER_POOL should be just

#define ENTER_POOL                      @autoreleasepool{

Larry Campbell <lcampbel>
Sat 10 Oct 2020 07:47:38 PM UTC, comment #7: 

Actually the proposed fix creates (incidentally) a bug that causes NSOperation threads never to exit!

-[NSOperationQueue _thread] (now) starts off like this (the ENTER_POOL/LEAVE_POOL were recently added as part of the fix here):

  for (;;)
    {
      /* We use a pool for each operation in case releasing the operation                                                          
       * causes it to be deallocated, and the deallocation of the operation                                                        
       * autoreleases something which needs to be cleaned up.                                                                      
       */
      ENTER_POOL
      NSOperation       *op;
      NSDate            *when;
      BOOL              found;

      when = [[NSDate alloc] initWithTimeIntervalSinceNow: 5.0];
      found = [internal->cond lockWhenCondition: 1 beforeDate: when];
      RELEASE(when);
      if (NO == found)
        {
          break;        // Idle for 5 seconds ... exit thread.                                                                     
        }

ENTER_POOL is a macro that expands to:

#define ENTER_POOL                      @autoreleasepool{do{

This is extremely dangerous!  The 'break' above in thread breaks from the do loop started by ENTER_POOL, not from the for(;;) loop in _thread, which is what was obviously intended, and what the code did before the ENTER_POOL was added in commit 41badcb417.

I really don't understand why ENTER_POOl isn't defined as

#define ENTER_POOL                      @autoreleasepool{do{

and LEAVE_POOL as

#define LEAVE_POOL                      }

This is safer, does not hijack 'break' and 'continue' statements between them, and everything compiles fine this way (except for Tools/AGSHtml.m, which is clearly confused about its break statement at line 1087).

Larry Campbell <lcampbel>
Wed 30 Sep 2020 09:15:10 AM UTC, comment #6: 

Having investigated, I think your problem is not a leak, but one of timing.

Your testcase waits for the operation to complete, but completion of an operation takes place before the operation is released/deallocated.  So completion of the operation in its thread can wake the main thread (which prints the retain count) before the object retain count is decremented giving the impression of a leak where none exists.
You can check this by adding a short delay to allow the other thread to release the object
eg.
   [NSThread sleepForTimeInterval: 0.1];
between calling -test and printing the retain count.

While looking at this, I did notice that if the operation autoreleases objects they will be caught by the outer autorelease pool of the thread and may persist until the thread ends.  So I changed the code in trunk to wrap execution of each individual operation in its own pool.

Richard Frith-Macdonald <CaS>
Project MemberIn charge of this item.
Wed 30 Sep 2020 08:06:36 AM UTC, comment #5: 

Thanks ... given the comment of the commit, it looks like this was an entirely accidental change.

Richard Frith-Macdonald <CaS>
Project MemberIn charge of this item.
Tue 29 Sep 2020 09:52:55 PM UTC, comment #4: 

Probably should restore the check for isCancelled, too.

Larry Campbell <lcampbel>
Tue 29 Sep 2020 09:50:40 PM UTC, comment #3: 

Looks like this bug was added by this commit:

commit 7380e850e4745282806e62320df5c016bfd4f31e
Author: Richard Frith-Macdonald <rfm@gnu.org>
Date:   Tue Dec 4 11:09:18 2018 +0000

    minor whitespace tidyups

The relevant part of that commit is:

@@ -973,11 +963,8 @@ - (void) _thread
            {
              NSAutoreleasePool *opPool = [NSAutoreleasePool new];

-             if (NO == [op isCancelled])
-               {
-                 [NSThread setThreadPriority: [op threadPriority]];
-                 [op main];
-               }
+              [NSThread setThreadPriority: [op threadPriority]];
+              [op start];

Changing "[op start]" back to "[op main]" fixes the leak.

Larry Campbell <lcampbel>
Tue 29 Sep 2020 12:44:23 PM UTC, comment #2: 

Another data point: the leak does NOT exist in gnustep-base 1.24.9. So it seems to have been introduced between 1.24.9 and 1.26.0.

Larry Campbell <lcampbel>
Tue 29 Sep 2020 12:40:06 PM UTC, comment #1: 

I discovered this in gnustep-base 1.27.0 but have verified that the leak also exists in gnustep-base 1.26.0.

Larry Campbell <lcampbel>
Tue 29 Sep 2020 12:39:15 PM UTC, original submission:  

The following code demonstrates that NSOperationQueue leaks operations. It should (and, on Mac OS X, does) print "t retain count is 1" at the end. On gnustep, it prints "t retain count is 2".

#import <Foundation/Foundation.h>

@interface Test : NSObject
@end

@implementation Test

- (void)frob
{
    NSLog(@"frob");
}

- (void)test
{
    NSAutoreleasePool *pool = [NSAutoreleasePool new];
    NSOperationQueue *opq = [[NSOperationQueue new] autorelease];
    NSInvocationOperation *op = [[[NSInvocationOperation alloc] initWithTarget:self selector:@selector(frob) object:nil] autorelease];
    [opq addOperation:op];
    [opq waitUntilAllOperationsAreFinished];
    [pool release];
}

@end

int main(int argc, char *argv[])
{
    NSAutoreleasePool *pool = [NSAutoreleasePool new];
    Test *t = [[Test new] autorelease];
    [t test];
    NSLog(@"t retain count is %lu", [t retainCount]);
    [pool release];
    return 0;
}

Larry Campbell <lcampbel>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-11 FredKiefer CategoryNone => Base/Foundation
        Item GroupNone => Bug
        StatusNone => Ready For Test
        Assigned toNone => CaS
        Open/ClosedAnalyzed => In Test
    2020-09-30 CaS Open/ClosedOpen => Analyzed

    Back to the top


    Powered by Savane 3.5