NSTask termination notifications can be lost or delayed

[lcampbel]

in -[NSTask waitUntilExit], after the loop we have to call -[NSRunLoop runMode:beforeDate:] twice: once to perform the _notifyOfTermination performer, which queues the notification, and again to dequeue and post the notification. Otherwise the following test fails:

static BOOL taskTerminationNotificationReceived;

- (void)taskDidTerminate:(NSNotification *)notification
{
    NSLog(@"Received NSTaskDidTerminateNotification %@", notification);
    taskTerminationNotificationReceived = YES;
}

- (void)testNSTaskNotifications
{
    NSDate *deadline;
    BOOL earlyTermination = NO;

    for (;;) {
        NSTask *task = [NSTask new];
        [task setLaunchPath:@"/bin/sh"];
        [task setArguments:[NSArray arrayWithObjects:@"-c", @"echo Child starting; sleep 10; echo Child exiting", nil]];
        taskTerminationNotificationReceived = NO;
        [[NSNotificationCenter defaultCenter] addObserver: self
                                                 selector: @selector(taskDidTerminate:)
                                                     name: NSTaskDidTerminateNotification
                                                   object: task];
        [task launch];
        NSLog(@"Launched pid %d", [task processIdentifier]);
        if (earlyTermination) {
            NSLog(@"Running run loop for 5 seconds");
            deadline = [NSDate dateWithTimeIntervalSinceNow:5.0];
            while ([deadline timeIntervalSinceNow] > 0.0)
        [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]];
            NSLog(@"Run loop finished, will now call -[NSTask terminate]");
            [task terminate];
            NSLog(@"Terminate returned, waiting for termination");
            [task waitUntilExit];
        } else {
            NSLog(@"Running run loop for 15 seconds");
            deadline = [NSDate dateWithTimeIntervalSinceNow:15.0];
            while ([deadline timeIntervalSinceNow] > 0.0 && !taskTerminationNotificationReceived)
                [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]];
        }
        [task release];
        NSAssert(taskTerminationNotificationReceived, @"termination notification not received");
        [[NSNotificationCenter defaultCenter] removeObserver:self name:NSTaskDidTerminateNotification object:nil];
        if (earlyTermination)
            break;
        earlyTermination = YES;
    }
}

Suggested fix:

*** NSTask.m~   Tue Jul  2 14:09:04 2024
--- NSTask.m    Wed Jul  3 12:36:36 2024
***************
*** 877,887 ****
      }
    [timer invalidate];

!   /* Run loop one last time (with limit date in past) so that any
!    * notification about the task ending is sent immediately.
     */
    limit = [NSDate dateWithTimeIntervalSinceNow: 0.0];
    [loop runMode: NSDefaultRunLoopMode beforeDate: limit];
    LEAVE_POOL
  }

--- 877,891 ----
      }
    [timer invalidate];

!   /* Run loop twice more (with limit date in past) so that any
!    * notification about the task ending is sent immediately. (Twice
!    * because the first time runs the _notifyOfTermination performer,
!    * which queues the notification, and the second dequeues and posts
!    * the notification.)
     */
    limit = [NSDate dateWithTimeIntervalSinceNow: 0.0];
    [loop runMode: NSDefaultRunLoopMode beforeDate: limit];
+   [loop runMode: NSDefaultRunLoopMode beforeDate: limit];
    LEAVE_POOL
  }

[lcampbel]

Hang on... this was my attempt at a cleaner-looking fix than my first, which did work, but this one also fails the test. Stay tuned...

[lcampbel]

This patch works:

*** NSTask.m~   Tue Jul  2 14:09:04 2024
--- NSTask.m    Wed Jul  3 13:44:56 2024
***************
*** 857,865 ****
    NSRunLoop     *loop = [NSRunLoop currentRunLoop];
    NSTimer *timer = nil;
    NSDate  *limit = nil;

    IF_NO_ARC([[self retain] autorelease];)
!   while ([self isRunning])
      {
        /* Poll at 0.1 second intervals.
         */
--- 857,866 ----
    NSRunLoop     *loop = [NSRunLoop currentRunLoop];
    NSTimer *timer = nil;
    NSDate  *limit = nil;
+   int            extra = 1;     // call run loop once more after termination so notifications are posted promptly

    IF_NO_ARC([[self retain] autorelease];)
!   while ([self isRunning] || extra-- > 0)
      {
        /* Poll at 0.1 second intervals.
         */

[rfm]

The more I look at this, the less sure I am about it.

Yes, there's a delay until the run loop posts the notification, but the Apple documentation doesn't say the notification is posted immediately (before -waitUntilExit completes). And I can't see how the notification can be lost other than by not running the run loop.

If the task was launched in another thread, the notification must be posted in that thread, so there's certainly no guarantee that the notification is delivered before -waitUntilExit ends in that case.

So it seems to me that, even if we want the undocumented behavior of the notification being delivered before -waitUntilExit returns, then the patches don't guarantee it when it was started in another thread.

So this rather looks like there is nothing terribly wrong with current behavior.

Still, I'm happy to add your patch (I can't see that it does any harm). I rather prefer the first version since, while it may be a little less elegent, it is very clear/explicit/simple with the comment.

Thoughts?

[lcampbel]

The problem is that the thread calling waitUntilExit may not have (use) a run loop. So if waitUntilExit doesn't cause the notification to be posted, and the thread never calls the run loop afterwards, it won't be posted.

• lc

On Jul 3, 2024, at 16:36, rfm @.***> wrote:

The more I look at this, the less sure I am about it.

Yes, there's a delay until the run loop posts the notification, but the Apple documentation doesn't say the notification is posted immediately (before -waitUntilExit completes). And I can't see how the notification can be lost other than by not running the run loop.

If the task was launched in another thread, the notification must be posted in that thread, so there's certainly no guarantee that the notification is delivered before -waitUntilExit ends in that case.

So it seems to me that, even if we want the undocumented behavior of the notification being delivered before -waitUntilExit returns, then the patches don't guarantee it when it was started in another thread.

So this rather looks like there is nothing terribly wrong with current behavior.

Still, I'm happy to add your patch (I can't see that it does any harm). I rather prefer the first version since, while it may be a little less elegent, it is very clear/explicit/simple with the comment.

Thoughts?

— Reply to this email directly, view it on GitHub https://github.com/gnustep/libs-base/issues/423#issuecomment-2207218956, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABY5W6JVBPPBQPNO5FZTLN3ZKROEDAVCNFSM6AAAAABKJ7C3TSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBXGIYTQOJVGY. You are receiving this because you authored the thread.

[rfm]

OK, so if the thread which launched the task calls -waitUntilExit the notification should be delivered before -waitUntilExit completes.