Is the order of operations threadsafe?

Question

I have an std::vector that is used to pass message to a thread. The vector is protected by a mutex:

vector<int> messages;
std::mutex m;

void postmessage (int msg) {
  {
    std::unique_lock Lock (m);
    messages.push_back (msg);
  }
}

When the thread is woken up, it grabs the entire message queue like this:

  const auto my_messages = [this] {
    std::unique_lock Lock (m);
    return move (messages);     
  } ();

It then processes my_messages. Questions:

Is this guaranteed to leave the vector in an empty state? In other words, is this an application of constructor (7) on cppreference that guarantees that the vector is left empty?
Are the operations on messages guaranteed to be completed before the mutex is unlocked? In other words, is this thread-safe?

This is scary to me because I don't know if the lambda returns `vector` or `vector &&`, and if it's the second one, *it will still work but it won't be thread-safe*. (I tested it and it's the first one, but I don't know why!) — user253751, Jul 14 '20 at 19:17
`auto` -> value or lvalue ref. `auto&&` -> lvalue ref or rvalue ref. Rvalue references do not "propagate" like that, specifically because it _is_ dangerous. That's why `std::forward` exists. — cdhowie, Jul 14 '20 at 19:30

cdhowie · Accepted Answer · 2020-07-14T19:23:26.813

3

Is this guaranteed to leave the vector in an empty state?

Yes, the returned value is move-constructed from messages, therefore messages is empty as you have correctly deduced.

This does not apply to all standard library types, so be careful -- some containers leave the source in an "unspecified but valid" state, which means it is only safe to perform actions on the source that have no preconditions. In those cases you can safely clear() the source to ensure that it is empty; clear() has no preconditions, and leaves the container empty.

Are the operations on messages guaranteed to be completed before the mutex is unlocked? In other words, is this thread-safe?

Yes, this is safe. It seems you are most worried about the code that "grabs" the message queue.

The crux of this issue is whether messages is used after Lock is destructed. The answer is no, it is not used.

Construction of the returned value must complete before any locals are destructed. Why? Because you might use a local to construct the return value!

std::vector<int> foo() {
    std::vector<int> x;
    return x;
}

x can't be destructed before the return value is constructed or there is undefined behavior.

edited Jul 14 '20 at 19:23

answered Jul 14 '20 at 18:50

cdhowie

158,093
24
286
300

Without wanting to argue, but in order for me to learn: Where in the standard is it specified that after `std::move(messages)`, `messages` goes to an empty state? (I read the answer as saying that it is guaranted for vectors) – Jeffrey Jul 14 '20 at 18:54
To elaborate more on the 2nd point - although the `mutex` is guaranteed to stay locked while the returned `vector` is being *constructed*, the `mutex` is unlocked when it goes out of scope *before* the caller is able to *use* the returned `vector`. – Remy Lebeau Jul 14 '20 at 18:55
@RemyLebeau Indeed, but we don't care about the returned vector. We only care if `messages` is used after the lock is released, and in this code sample it isn't. – cdhowie Jul 14 '20 at 18:56
@Jeffrey `std::vector`'s move constructor is implemented to ensure that `messages` will be in an empty state after the returned `vector` object is constructed from `messages`. The **standard** doesn't dictate that a moved-from container will be "empty", only that it be left in a "valid but indeterminate state" after being moved from, but for `std::vector` specifically, it is usually implemented in such a way that the moved-from state is "empty". – Remy Lebeau Jul 14 '20 at 18:56
Why is the return type `vector` and not `vector &&`? Are return types always deduced as value types by default? – user253751 Jul 14 '20 at 19:17
I personally would not rely on 'usually'. – Jeffrey Jul 14 '20 at 19:19
1

@Jeffrey See the link I added to the first paragraph. tl;dr is that `vector(vector&&)` is specified to have O(1) complexity and there is no other way to achieve that. – cdhowie Jul 14 '20 at 19:23
@user253751 From what I recall, `auto` is always either a value or an lvalue reference; `auto&&` is always either an lvalue reference or an rvalue reference. Return type deduction follows the same rules as template type deduction. – cdhowie Jul 14 '20 at 19:26
@cdhowie :cheers: that works for me. It has enough gory details that if it were my code, I'd still waste a bit to explicitly clear() the vector, but also enough meat to justify relying on it being empty. – Jeffrey Jul 14 '20 at 19:27
@RemyLebeau • `std::vector` is required to have its moved-from state be empty and in a valid state. (I don't have chapter-and-verse on hand.) But in general, it's good policy to presume anything moved-from (even if the move-operation didn't take place) is in a valid-but-indeterminate state only good for re-initializing or destructing. Makes code reviews a lot less nitpicky. – Eljay Jul 14 '20 at 20:15
@Eljay I scoured the C++14 specification for such a declaration and found none. The requirement is implied from its time complexity constraint only. The spec doesn't explicitly say that a moved-from vector is empty. Indeed, I don't think a vector used for a _move-assignment_ source is required to be empty; likely most implementations will just swap in the move-assignment case. – cdhowie Jul 15 '20 at 00:01
@cdhowie • https://en.cppreference.com/w/cpp/container/vector/vector note #7 mentions after the move, the moved-from is empty. I'm not sure what that corresponds to in the standard for chapter-and-verse, but I presume they didn't make it up (and it was also mentioned by my co-worker, and he's rather well-versed in the standard). – Eljay Jul 15 '20 at 01:12
@Eljay It's not explicitly stated in the standard, but it's not made up either: it's inferred from the time complexity constraint. – cdhowie Jul 15 '20 at 01:53

Is the order of operations threadsafe?

1 Answers1