Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I think I've identified a race condition #25

Closed
ghost opened this issue Aug 3, 2014 · 7 comments
Closed

I think I've identified a race condition #25

ghost opened this issue Aug 3, 2014 · 7 comments
Assignees
Labels
Milestone

Comments

@ghost
Copy link

ghost commented Aug 3, 2014

I've been having trouble when disconnecting an FTDI based USB device. Occasionally, unplugging the device messes up my USB bus and 'disconnects' Xcode's debugger from my code (I can neither stop or pause my code), making this a particularly nasty problem to debug. Also, I usually have audio from iTunes playing through a USB interface and that stops as well. The only thing that clears this up is restarting the machine, which I sometimes have to do via Terminal.

I've tracked this down to what I believe is a race condition.

When a device is unplugged, eventually -[ORSSerialPort close] is called on the main thread. Inside -[ORSSerialPort close] are calls to tcsetattr().

Meanwhile in another thread, a while loop executing in the read poller calls select() on each iteration.

Using printf() debugging, I have come to believe that if select() is called while a call to tcsetattr() is in flight (or vice-versa), some sort of deadlock occurs.

My fix for this is to wrap the critical parts of -[ORSSerialPort close] in a @synchronized block

- (BOOL)close;
{
    BOOL result;

    if (!self.isOpen)
    {
        result = YES;
    }
    else
    {
        @synchronized(self) {

            [self.writeBuffer replaceBytesInRange:NSMakeRange(0, [self.writeBuffer length]) withBytes:NULL length:0];

            // The next tcsetattr() call can fail if the port is waiting to send data. This is likely to happen
            // e.g. if flow control is on and the CTS line is low. So, turn off flow control before proceeding
            struct termios options;

            tcgetattr(self.fileDescriptor, &options);
            options.c_cflag &= ~CRTSCTS; // RTS/CTS Flow Control
            options.c_cflag &= ~(CDTR_IFLOW | CDSR_OFLOW); // DTR/DSR Flow Control
            options.c_cflag &= ~CCAR_OFLOW; // DCD Flow Control
            tcsetattr(self.fileDescriptor, TCSANOW, &options);

            // Set port back the way it was before we used it
            tcsetattr(self.fileDescriptor, TCSADRAIN, &originalPortAttributes);

            int localFD = self.fileDescriptor;

            self.fileDescriptor = 0; // So other threads know that the port should be closed and can stop I/O operations

            if (close(localFD))
            {
                result = NO;
                self.fileDescriptor = localFD;
                LOG_SERIAL_PORT_ERROR(@"Error closing serial port with file descriptor %i:%i", self.fileDescriptor, errno);
                [self notifyDelegateOfPosixError];
            }
            else
            {
                result = YES;
                if ([(id)self.delegate respondsToSelector:@selector(serialPortWasClosed:)])
                {
                    [self.delegate serialPortWasClosed:self];
                }
            }
        }
    }

    return result;
}

Similarly, I wrap the read poller's call to select() in a @synchronized block:

// Start a read poller in the background
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
    while (self.isOpen) 
    {
        ...
        @synchronized(self){
            result = select(localPortFD+1, &localReadFDSet, NULL, NULL, &timeout);
        }
        ...
    }
    ...
});

I've not had a USB lock up when disconnecting my device since making this change.

@armadsen
Copy link
Owner

armadsen commented Nov 3, 2014

Thanks for identifying this. I'm going to have to try to reproduce it myself. I'd rather not use @synchronized if I can avoid it. I'd favor an approach that used GCD to avoid the deadlock instead.

@armadsen armadsen added this to the 1.7.0 milestone Mar 8, 2015
@armadsen armadsen self-assigned this Mar 8, 2015
@armadsen armadsen added the bug label Mar 8, 2015
@armadsen
Copy link
Owner

I'm unable to reproduce this with a RFDuino, which uses an FTDI chip for its USB interface. printf() debugging does indicate that one of the tcsetattr() calls in -close often collides with the read poller's select() but it doesn't seem to deadlock. Will keep an eye out...

@armadsen armadsen modified the milestones: 1.7.0, Postponed Mar 15, 2015
@ghost
Copy link
Author

ghost commented Mar 15, 2015

Keep trying! The deadlock behavior I observed was hard to reproduce, but hasn't occurred since my fix. Also, I had multiple USB devices connected. Perhaps that's a differentiator.

@armadsen
Copy link
Owner

I was finally able to reproduce this today using a Prolific USB to serial adapter. I'm still not able to reproduce it with a Keyspan or FTDI adapter, so I think it may be driver dependent.

armadsen pushed a commit that referenced this issue Mar 23, 2015
@armadsen
Copy link
Owner

OK, I've synchronized calls to select() in the read poller, and the POSIX API calls in -close. This does seem to prevent the deadlock. I did notice that the second call to tcsetattr() in -close, as well as the close() call itself can hang indefinitely with Prolific's adapters. I think it's just a bug in their driver (Prolific's drivers have always been notoriously terrible.) It's obviously bad, as it's a hang, but it's not as bad as this deadlock in that killing the app doesn't seem to cause any ill effects, and in fact, interrupting with the debugger then continuing will "unstick" it.

@ghost
Copy link
Author

ghost commented Mar 26, 2015

Hey, were you testing with FTDI's drivers or the one built into Mac OS X? I am running with FTDI's drivers (FTDIUSBSerialDriver.kext, 2.2.18) because Apple's built-in driver doesn't allow programmatic toggling of the DTR line.

It's with the FTDI driver that I had the deadlocks. I don't think I ever deadlocked using Apple's driver, but then again, I didn't use it that much.

@armadsen
Copy link
Owner

Good point. I was testing with the built in drivers. I'll have to try with the FTDI drivers (just to confirm). Weird and annoying that Apple's driver doesn't allow toggling the DTR line. I didn't know that, and it's useful information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant