jump to case label crosses initialization of 'std::unique_lock'

Question

I am learning thread synchronization between two threads using condition variable but getting this compilation error:

CVV.cpp:21:3: error: jump to case label [-fpermissive]
   default:
   ^
CVV.cpp:14:33: error:   crosses initialization of ‘std::unique_lock<std::mutex> ul’
    std::unique_lock<std::mutex> ul(m, std::defer_lock);

This is my code

#include <iostream>
#include <thread>
#include <mutex>
#include <condition_variable>

std::mutex m;
std::condition_variable cv;

void recv_data() {
    int32_t opn_type;
    
    switch(opn_type) {
        case 1:
            std::unique_lock<std::mutex> ul(m);
            // do processing and add to queue
            cv.notify_one();
            ul.unlock();
            
            break;
            
        default:
            break;
    }
}

int32_t main() {
    std::thread th(recv_data);
    th.join();
    
    return EXIT_SUCCESS;
}

Can somebody explain what I'm doing wrong

Answer 1

switch(opn_type) {
    case 1: {
        std::unique_lock<std::mutex> ul(m);
        // do processing and add to queue
        cv.notify_one();
        ul.unlock();
        
    }   break;
        
    default:
        break;
}

case labels don't end variable lifetime, so ul exists and is destroyed after default: but only initialized within case 1:.

Put it in a block.

switch(opn_type) {
  case 1: {
    std::unique_lock<std::mutex> ul(m);
    // do processing and add to queue
    cv.notify_one();
  }   break;
    
  default:
    break;
}

the .unlock() does nothing, as the scope is ending right there. In this version I removed it.

Note that I find mixing threading primitives with other code to be dangerous.

template<class T>
struct threadsafe_queue {
  T pop() {
    auto l = lock();
    cv.wait(lk, [&]{return !queue.empty();});
    T retval = queue.front();
    queue.pop_front();
    return retval;
  }
  void push( T in ) {
    auto l = lock(m);
    queue.push_back(std::move(in));
    cv.notify_one();
  }
  std::deque<T> pop_all() {
    auto l = lock();
    return std::move(queue);
  }
private:
  mutable std::mutex m;
  std::condition_variable cv;
  std::deque<T> queue;
  std::unique_lock<std::mutex> lock() const {
    return std::unique_lock<std::mutex>(m);
  }
};

adding "try_pop", "try_pop_for" etc is an exercise.

Answer 2

I think the root of the issue here is understanding the error message. Let's dissect it:

CVV.cpp:21:3: error: jump to case label [-fpermissive]
   default:
   ^
CVV.cpp:14:33: error:   crosses initialization of ‘std::unique_lock<std::mutex> ul’
    std::unique_lock<std::mutex> ul(m, std::defer_lock);

Note that the 2nd error message is indented: the word crosses is farther in than the word jump. This is not has meaning, although admittedly that meaning may not be super-obvious. The indentation implies a continuation of the message, i.e. you must read it together with preceding messages, starting from the nearest non-indented message. The compiler is indicating that the error is spread across several related locations. It's not two errors, but one error that provides well-needed detail. The message must be read continuously. And thus, it's not a "crosses initialization of" error. It's a "jump to case label crosses initialization of" error. Now that starts making some sense, I hope :)

Here's how to read it:

In CVV.cpp, line 21, there's an error caused by jumping to case label default:,
because that jump crosses an initialization of ul made in line 14.

Recall that switch acts more or less like:

if (argument == case1) goto case1;
else if (argument == case2) goto case2;
...
else goto default;

So, since switch has to jump from switch to the default: label, it crosses an initialization. So all you need to fix the error is to get rid of the initialization :)

But "getting rid of it" doesn't need to be literal. The problem you are trying to solve is that ul is visible at the default label, but when you jump to that label, the initialization for ul hasn't run, since you jump over it, yet ul is in scope, i.e. accessible for any code in the default: case. And thus that code could potentially use an uninitialized object. But that's only half the problem. The non-existent ul object can only be destroyed when it's no longer in scope, and in the default: section it still is in scope. Thus the destruction will run afterwards, without there ever being a construction, and that's incorrect and undefined behavior, too.

One solution then is to reorder the cases, so that in no case is there a jump over ul's initialization - simply put it last:

switch(opn_type) {
    default:
        break;
    case 1:
        std::unique_lock<std::mutex> ul(m);
        // do processing and add to queue
        cv.notify_one();
        ul.unlock();            
        break;            
}

That's not always possible, of course. It's just an example to make it clear that the most "obvious" solution would have worked as well.

A more general solution, then, is to insert additional scope by wrapping the code in blocks:

switch(opn_type) {
    case 1: {
        std::unique_lock<std::mutex> ul(m);
        // do processing and add to queue
        cv.notify_one();
        ul.unlock();            
        break;
    }            
    default:
        break;
}

In this case, the jump to default: is fine, since at that point ul is out of scope, i.e. not visible, and thus there'll be no possibility of accessing an uninitialized object, nor of attempting to destruct and object that doesn't exist (synonym for "hasn't been constructed"). The destruction will happen only when the case 1: block has been entered first, and will happen at the closing brace.

I'm not addressing anything to do with threading primitives here - this answer is meant to be generic.