0

I have a QThread running some code and I wanted it to do exit nicely and do some cleanings so the code goes as:

testdevice.h

class testDevice : public QThread 
{
    Q_OBJECT
... // some definitions 

protected:
    void run(void);

private:
    hid_device *handle;
    bool abort;

public:
    ~testDevice(void);
};

testdevice.cpp

testDevice::~testDevice(void)
{    
    mutex.lock();
    abort = true;
    mutex.unlock();
    wait();

    if (handle != NULL)
    {
        hid_close(handle);
    }

    hid_exit();
}

void testDevice::run(void)
{    
    while(true)
    {        
        mutex.lock();
        if(abort)
        {
           return;
        }
       mutex.unlock();
       if (_connected)
       {
        //... more code
       }
       else // Case device is not connected
       { 
       emit deviceConnected(_connected);// A breakpoint in here is not triggered after abort is true
       handle = hid_open(TEST_VID, TEST_PID, NULL); // this is where VS2010 shows the error "Unhandled exception at 0x71241a95 in project.exe: 0xC0000005: Access violation."
       //...code continues

The behaviour I was expecting was run() to be called and when ~testDevice() destructor is called abort is set to true and wait blocks the destructor, and runreturns and then the destructor continues.

What is happening is that run() is called and when I close the application the destructors ~testDevice() is called and abort is set to true and wait() returns and the destructor finishes...BUT then run() keeps running and I get Unhandled exception at 0x71241a95 in project.exe: 0xC0000005: Access violation.....I am running this as VS2010 debug, if I place breakpoints I get this all the time, but with no breakpoints I just got this once in a while...any clues?

Something weird is that when I place a breakpoint in abort = true; sometimes the first breakpoint that hits in there has a small blue '!' sign, and the second time it hits is a regular all red one. The same happens for the breakpoint at emit deviceConnected(_connected); but this is kinda random as well...I dont know what this '!' means...could all of this be a debug problem?

My general suspicion is that HIDAPI is running in a separate thread and it has its own bugs when someone call hid_exit(); maybe hid_read(); continues to run and since hid_quit(); was called, hid_read(); looses some pointers and doesnt close...just a wild shot and to confirm this I need some developer at the HIDAPI to come foward and say something...I hope they read this

László Papp
  • 51,870
  • 39
  • 111
  • 135
Michel Feinstein
  • 13,416
  • 16
  • 91
  • 173
  • Read about thread safety, synchronization (and atomics) - you're reading and modifying `abort` with two different threads without synchronization => undefined behavior. – Mat Dec 27 '13 at 21:46
  • @LaszloPapp: whether it's assigning a constant value or an elephant has nothing to do with it. The definition of a data race (in C++11 and C11) is (simplified) "two thread access the same (non-atomic) object, and at least one of them does a write". There is no guarantee here that the write to `abort` in one thread will be visible to the other, it's undefined behavior. – Mat Dec 28 '13 at 07:42
  • @LaszloPapp: from what I understsand of the question, OP is calling the destructor of one specific QThread when they close the app. That specific destructor will (presumably) run from the main thread, and attempts to have an effect on the thread that runs that specific QThread's `run()` method but setting `abort`. That's "illegal". Obviously there's no thread safety issues if there's no shared data. – Mat Dec 28 '13 at 07:56
  • @Mat: but he waits for the thread to quit after setting the abort. So what is the problem there about synchronization? The OP seems to have done that. Note that, the abort member data is specific to a thread, so there is no sharing between different threads (QThread instances). – László Papp Dec 28 '13 at 08:01
  • @LaszloPapp: `abort` is a plain bool, not an atomic. The compiler that digests the `run` function can cache it's value after noticing that there's no writes to it in the body in this case. The `wait` after setting it doesn't help since there's not synchronization at all in `run` (i.e. nothing that tells the compiler it has to reload the value of `abort`). `volatile` will appear to work here, at least on x86, but that's a bad hack. The reads and writes to `abort` need to be protected (or abort made atomic) for this to work. Or `run` needs to synchronize with the destructor somehow. – Mat Dec 28 '13 at 08:06
  • @Mat: you are claiming that the compiler will not optimize out an std::atomic variable? I agree that volatile is a nasty workaround here. – László Papp Dec 28 '13 at 08:09
  • @LaszloPapp: a compiler **cannot** optimize out the effects of atomic reads and writes wrt to synchronization, that's the whole point of atomics. – Mat Dec 28 '13 at 08:10
  • @Mat: OK, I will incorporate this into my answer, thanks. By the way, why do you think "volatile" will only work on x86? I think it works elsewhere, too, including e.g. ARM, et al. In fact, that is the realm of similar situations in C including the arm platform. :-) Admittedly, it was invented for interrupts, gpios, etc, not really for data race solutions... – László Papp Dec 28 '13 at 08:13
  • @LaszloPapp: see the link I posted under phyatt's answer and the related search term. `volatile` is not a synchronization primitive, it was designed for low-level hardware access. It'll "work" in certain cases for certain data types on certain CPUs (x86 has very strong coherency guarantees, ARM much less). (Herb Sutter's "Atomic<> Weapons" talk at Going Native 2013 (avail. on Channel9) is a good watch if you have the time/interest.) – Mat Dec 28 '13 at 08:19
  • Yes, I agree as written, and I already modified my answer respectively. Feedback would be welcome, thanks. – László Papp Dec 28 '13 at 08:23
  • I edited some stuff, please take a look – Michel Feinstein Dec 28 '13 at 23:35
  • -1. The OP radically changed the original post again after people giving right answers. I vote for closing it. The OP keeps doing this in many threads all around. This is unfortunate. – László Papp Dec 29 '13 at 10:43
  • I edit the code when the answers point to things that dont actually solves the problem. This way furure readers wont think that was the problem. Once the answer is finnaly answered I leave the code with the mistake that has to be corrected for future reference. This way small mistakes that creeps into the code doesnt influenciate the question, specially for furure people that will read it for trying to answer or read. So at the end the code will be only with parts that were related to the bug itself. – Michel Feinstein Dec 29 '13 at 10:47
  • @mFeinstein: please provide a self-contained example with the atomic usage so that we can see it really did not solve your issue. – László Papp Jan 08 '14 at 13:18
  • Laszlo, I am still having issues when closing the program, but I have changed my code a lot, later when things are more finished I will open a new post in here because the code and the question will be a lot different – Michel Feinstein Jan 08 '14 at 13:32
  • @mFeinstein: changing all my downvotes into upvotes on your questions. ;) – László Papp May 31 '14 at 05:46
  • @FinalContest Yes I just saw it...I wonder what I did to deserve such a change in judgment... – Michel Feinstein May 31 '14 at 21:13
  • I wrote up a good answer to this in the past: http://stackoverflow.com/questions/19388207/how-to-stop-thread-qthread/19394348#19394348 Basically, you use a `volatile bool` instead of a `bool` and the compiler doesn't over optimize your logic. Or you can look into some of the nice thread synchronization stuff Qt comes with. Hope that helps. – phyatt Dec 27 '13 at 22:08
  • Is this still unresolved one year later?? – László Papp Dec 21 '14 at 10:41

1 Answers1

1

You seem to be using QThread in a custom way rather than the usual start() and quit() execution. The best would be that in an ideal world if you could refactor your code using quit() for quiting a QThread since that would be closer to the signal/slot mechanism of this QObject subclass.

However, I agree that it is not always possible. I cannot yet tell you based on your few lines you provided that it would make sense in this particular case, but at least I would encourage you to think about it.

Also, you could possibly use the QWaitCondition for waiting for something to happen rather than low-level or/and custom wait() calls.

As for quickly working around the issue you are facing if the issue at hand is compiler optimization of a boolean value like putting it into the cache, you could try having making it atomic by using the std::atomic (C++11) or QAtomicInt option. This will make sure that the compiler cannot optimize the reads and writes out.

You would write something like this with the std typedef:

std::atomic_bool abort;

If you need to support pre C++-11 compilers as well as Qt 4, you would use the Qt class like this:

QAtomicInt abort;
László Papp
  • 51,870
  • 39
  • 111
  • 135