bugGNUstep - Bugs: bug #59204, NSOperationQueue leak

Group
 
 

bug #59204: NSOperationQueue leak

Submitter:  Larry Campbell <lcampbel>
Submitted:  Tue 29 Sep 2020 12:39:15 PM UTC
   
 
Category:  Base/Foundation Severity:  3 - Normal
Item Group:  Bug Status:  Fixed
Privacy:  Public Assigned to:  CaS
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 08 Dec 2021 11:54:03 PM UTC, comment #15: 

No news since a year, I assume nothing bad was broken.

Riccardo Mottola <rmottola>
Group Member
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>
Group Member
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>
Group Member
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>
Group 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>
Group 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>
Group Member
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>
Group Member
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>
Group Member
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 rmottola (Posted a comment)
  • -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)
  •  

    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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-12-08 rmottola StatusReady For Test Fixed
        Open/ClosedIn Test Closed
    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.13-f8d8.
    Corresponding source code